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

Invalid command usage result code #582

Closed
TerraTech opened this issue Nov 19, 2017 · 13 comments
Closed

Invalid command usage result code #582

TerraTech opened this issue Nov 19, 2017 · 13 comments
Labels
area/shell-completion All shell completions

Comments

@TerraTech
Copy link

I may be missing something obvious, but say you have the command structure:
myapp foo bar

If I type:
myapp foo

It will return the normal Usage output showing (snipped for brevity): Available Commands: bar

This is fine, but in a shell script, I don't see how to catch the error cleanly because triggering Usage has a result code of 0, instead of a desired >=1.

myapp foo
if (( rc=$? )); then
  echo "[FAIL] myapp failed with rc==${rc}"
fi

This came up because one of the go apps that I use had foo, but now (from an upgrade) it has been extended to foo bar and I didn't realize it until my administrative script started to fail unexpectedly. It should have stopped dead at the myapp foo point, but an error didn't pop up until much later in the code.

@n10v n10v added the area/shell-completion All shell completions label Nov 19, 2017
@stverhae
Copy link

I'm encountering the same issue. A command was renamed but the go program simple prints the usage and considers it a day (exit code 0).
The shell script the program was used in is not able to detect this which is very problematic ...

If this behaviour is not getting changed very soon, what would be the easiest way to implement a temporary fix?

@jmalloc
Copy link

jmalloc commented Jan 18, 2018

I'm not sure if I'm missing something, but is this not just a matter of exiting with a non-zero code when Cobra returns an error?

if err := root.Execute(); err != nil {
    os.Exit(1)
}

This is probably better than having Cobra unconditionally perform os.Exit(1) itself.

@TerraTech
Copy link
Author

@jmalloc The issue is calling out to a go binary that has had one of its commands extended by an outside party. Cobra invoking the usage emittance routines, by definition will exit out. My request is for it to os.Exit(1) instead of os.Exit(0) so that scripts can catch the error condition and handle it accordingly. In a nutshell, this should be wired in at Cobra's usage exit.

@dnephin
Copy link
Contributor

dnephin commented Jan 18, 2018

myapp foo (or generally asking for usage) should not be an error. The command ran successfully.

You can make it an error by adding a RunE() and returning some error.

@TerraTech
Copy link
Author

TerraTech commented Jan 20, 2018

@dnephin without having access to the go binaries source code, and tripping over the usage issue, how would you best recommend handling this situation in a shell script? I'm not particularly inclined to capture the command's output and grepping it for a usage output.

I'm thinking that if you specifically invoke --help, then it should return success, but if you run a command that automatically trigger's usage output, that should return an exit other than 0 to make shell scripting easier when working with go binaries.

If you don't want to wire it in by default, how about an environment variable or option switch that can be passed, which is global to cobra and will provide exit >0 on non-manual invoked usage requests?

As it stands now, this is really the only thing that works: output=$(goBinary foo 2>&1); if grep -qi usage <<<"${output}"; then handle_error; fi

I don't mind code bases with a pedantic philosophy, but I'm sure many people would rather it be:
if ! goBinary foo; then handle_error; fi

@jmalloc
Copy link

jmalloc commented Jan 20, 2018

I'm thinking that if you specifically invoke --help, then it should return success, but if you run a command that automatically trigger's usage output, that should return an exit other than 0 ...

👍 This makes sense to me. If you didn't ask for help, then usage is displayed because you didn't provide enough/valid arguments. FWIW, there is precedent here, this is exactly how git and brew behave.

@dnephin
Copy link
Contributor

dnephin commented Jan 22, 2018

without having access to the go binaries source code,

How would you apply this fix if you don't have access to the source code? I don't understand.

if you don't want to wire it in by default

I was saying you can already have it return an error by setting a RunE()

@TerraTech
Copy link
Author

@dnephin People that are running a pre-compiled go binary that don't have access to the source to change it to RunE() and rebuild. Yes, we can bug the vendor to make the changes and cut a new binary, however I still feel that this should be better handled by cobra itself with its usage behavior.

What we control is the shell script, and not the go binary itself. Since cobra is the defacto go command processor, many 3rd parties use it when writing their apps therefore I believe that cobra should perform sane usage handling that augment error handling in scripts that call out to these cobra enhanced go binaries.

@jmalloc Well said and I consider that to be sane behavior that scripts can tap into for better error handling - e.g. wrong arguments or catching a typo. The script should stop dead at that point, instead of blindingly continuing on. :)

@bruceadowns
Copy link
Contributor

My PR subsumes PR submitted by @chriswhelix.

@umarcor
Copy link
Contributor

umarcor commented Sep 5, 2019

@bruceadowns, should this be closed because of #922?

@bruceadowns
Copy link
Contributor

@bruceadowns, should this be closed because of #922?

Yes.

@github-actions
Copy link

This issue is being marked as stale due to a long period of inactivity

@johnSchnake
Copy link
Collaborator

Based on the last few comments, closing this issue. There are others related to exit codes, but in different aspects presumably.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/shell-completion All shell completions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants