Skip to content

Add exit status to docker exec manpage - #1674

Merged
vdemeester merged 1 commit into
docker:masterfrom
ericcurtin:patch-1
Mar 13, 2019
Merged

Add exit status to docker exec manpage#1674
vdemeester merged 1 commit into
docker:masterfrom
ericcurtin:patch-1

Conversation

@ericcurtin

@ericcurtinericcurtin commented Feb 15, 2019

Copy link
Copy Markdown
Contributor

There's little way of knowing what each exit status means at present
because it's not documented. I'm assuming they are the same as
docker run.

@GordonTheTurtle

Copy link
Copy Markdown

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "patch-1" git@github.com:ericcurtin/cli.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@codecov-io

codecov-io commented Feb 15, 2019

Copy link
Copy Markdown

Codecov Report

Merging #1674 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@ Coverage Diff @@## master #1674 +/- ##
==========================================
+ Coverage 56.12% 56.12% +<.01% 
==========================================
Files 306 306 Lines 21025 20964 -61 ==========================================
- Hits 11800 11766 -34 + Misses 8371 8345 -26 + Partials 854 853 -1

@thaJeztahthaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thaJeztah

Copy link
Copy Markdown
Member

ping @vdemeester@silvin-lubecki ptal

Comment threadman/src/container/exec.md Outdated
$ docker exec --foo busybox; echo $?
# flag provided but not defined: --foo
See 'docker exec --help'.
125

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this example of an error within the Docker CLI, rather than the daemon, it's the CLI which isn't recognising the --foo here isn't it?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question; will have to dig into the history of that. Perhaps this was done on purpose to being able to distinguish a exit 1 (from the container's process) from an exit 1 (from the docker cli (or daemon))?

@ericcurtinericcurtinFeb 22, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add a different example if you guys have a common one off the top of your heads! I'm away for the weekend now, can try and recreate a 125 when I get back.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if another example is possible (as in; I think the 125 is generated in the CLI, as it won't send the flag to the daemon if it's invalid 🤔

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will I remove it for the moment?

There's little way of knowing what each exit status means at present
because it's not documented. I'm assuming they are the same as docker
run.
Signed-off-by: Eric Curtin <ericcurtin17@gmail.com>
@ericcurtin

Copy link
Copy Markdown
ContributorAuthor

Removed 125 exit status from man page

@ericcurtin

Copy link
Copy Markdown
ContributorAuthor

Poke poke @ijc@thaJeztah@vdemeester@silvin-lubecki

I removed 125. Is this good to go?

ijc
ijc approved these changes Mar 8, 2019

@ijcijc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (not a maintainer)

I think we should probably be accurately describing what 125 means rather than omitting it, but this is adding correct and useful information so is a worthwhile improvement and not worth blocking over "perfection".

@ericcurtin

Copy link
Copy Markdown
ContributorAuthor

@thaJeztah@vdemeester@silvin-lubecki any of you guys want to grab?

@vdemeestervdemeester left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🐯

@vdemeester
vdemeester merged commit c748c85 into docker:masterMar 13, 2019
@GordonTheTurtleGordonTheTurtle added this to the 19.03.0 milestone Mar 13, 2019
@ericcurtin
ericcurtin deleted the patch-1 branch March 20, 2019 18:20
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@ericcurtin@GordonTheTurtle@codecov-io@thaJeztah@vdemeester@ijc