Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lots of regression in exit codes [from 1.12 to master]rr #26152

Closed
runcom opened this issue Aug 30, 2016 · 6 comments
Closed

Lots of regression in exit codes [from 1.12 to master]rr #26152

runcom opened this issue Aug 30, 2016 · 6 comments
Assignees
Labels
kind/bug Bugs are bugs. The cause may or may not be known at triage time so debugging may be needed. version/master version/1.12
Milestone

Comments

@runcom
Copy link
Member

runcom commented Aug 30, 2016

Under docker-1.10, flag.go exits 125 when a command-line flag is missing a required argument. With docker 1.12 most commands do so... except for a seemingly random few:

   # docker load --input;echo $?
   flag needs an argument: --input
   See 'docker load --help'.
   1    <-- 125 with docker-1.10

   # docker ps --filters;echo $?         <-- same with -n or --format
   unknown flag: --filters
   See 'docker ps --help'.
   1    <-- 125 with docker-1.10

(List is not meant to be exhaustive; there are more).

If my diagnosis is correct, root cause is that as part of the cobra migration in June some NewXxxCommand() functions were added with this line:

    cmd.SetFlagErrorFunc(flagErrorFunc)

...and some were not:

    $ grep -L lagErrorFunc api/client/{image,container}/* | wc -l
    23

If this analysis is correct, I hope there's a less duplicationy way to solve this than adding cmd.SetFlagErrorFunc() to each source file!

@vdemeester @dnephin

@runcom runcom added kind/bug Bugs are bugs. The cause may or may not be known at triage time so debugging may be needed. version/master version/1.12 labels Aug 30, 2016
@dnephin
Copy link
Member

dnephin commented Aug 30, 2016

SetFlagErrorFunc() should only be necessary on the root command, not every command. I removed it from all the commands in #25354

I just tried the examples with a binary built on master and they all return the correct 125. Can you try master and see if it's fixed?

@runcom runcom changed the title Lots of egression in exit codes [from 1.12 to master] Lots of regression in exit codes [from 1.12 to master]rr Aug 30, 2016
@runcom
Copy link
Member Author

runcom commented Aug 31, 2016

@dnephin tried master, works fine, closing (thx)

@runcom runcom closed this as completed Aug 31, 2016
@thaJeztah thaJeztah added this to the 1.13.0 milestone Sep 21, 2016
@thaJeztah
Copy link
Member

ping @dnephin @vdemeester @runcom I checked this issue with 1.11, and it looks like this was a regression in 1.12. Do you think it's possible (and worth it) to fix just this issue for 1.12.2 (without including the conversion of docker to Cobra)?

@thaJeztah
Copy link
Member

Also ping @vieux ^^

@runcom
Copy link
Member Author

runcom commented Sep 21, 2016

ping @dnephin @vdemeester @runcom I checked this issue with 1.11, and it looks like this was a regression in 1.12. Do you think it's possible (and worth it) to fix just this issue for 1.12.2 (without including the conversion of docker to Cobra)?

this is huge and I don't have any clue if we can fix just this for 1.12.2 - would be great though if anyone can do this fast for 1.12.2 :)

@thaJeztah
Copy link
Member

@runcom I opened #26775 so that we don't loose sight of the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Bugs are bugs. The cause may or may not be known at triage time so debugging may be needed. version/master version/1.12
Projects
None yet
Development

No branches or pull requests

4 participants