-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Docs needed: how best to handle errors in Run #914
Comments
To suppress the usage, use the However, I totally agree with you on that it needs some kind of documentation. |
Any update on this topic please ? I am also looking at good practices about errors handling in Cobra. The |
After reading the Command code for a while, I don't see a really flexible way to decide how to handle errors at runtime. This is the approach we've ended up on: func main() {
if err := command.RootCmd.Execute(); err != nil {
if err != command.SilentErr {
fmt.Fprintln(os.Stderr, err)
}
os.Exit(1)
}
} package command
var SilentErr = errors.New("SilentErr")
var RootCmd = &cobra.Command{
// ...
SilenceErrors: true,
SilenceUsage: true,
}
func init() {
RootCmd.SetFlagErrorFunc(func(cmd *cobra.Command, err error) error {
cmd.Println(err)
cmd.Println(cmd.UsageString())
return SilentErr
})
} After this, we use The features of this approach are:
Drawbacks:
Ref. #340 |
This issue is being marked as stale due to a long period of inactivity |
Without SilenceUsage, errors in RunE also produce usage, so we have to make our own wrapper that prints errors without usage. See also: spf13/cobra#340 spf13/cobra#914 Closes #159
Without SilenceUsage, errors in RunE also produce usage, so we have to make our own wrapper that prints errors without usage. See also: spf13/cobra#340 spf13/cobra#914 Closes #159
…Cmd failures Based on the conv in suborbital#119, we want to delete the output dir of `sudo create runnable` for any failure that occurs in `CreateRunnableCmd`. `CreateRunnableError`s `Error` will also act as a cleanup function for these failures. atm, it's only deleting the output dir. Ideally, cobra.Command would have a error callback feature... maybe we'll get one for Xmas. spf13/cobra#914
I'd also like to run a |
Reading the docs, I thought RunE would set the error code on exit for me. I did not expect it to print usage info. It is hilarious to have my service exit with an error (because of misconfig), and then, rather than print that error, it prints the usage info. |
Different implementations may differ about exit codes to use, presumably thats why its left out of the implementation. Just as easy as it would be to configure a field I agree that it wouldn't be obvious that the postRun methods wouldn't be run if an error occurs. That does seem like something that would be useful to be configurable. You'd wan't the error from the I've seen that pattern used before; e.g.
Thoughts on that approach? |
I wanted to bring this up once again to see if anyone had made any progress or had thoughts on how to run Currently, in both the Did anyone succeed in finding workarounds for this? Because I have so many subcommands, it seems like bad design to have each of the subcommands contain some logic to perform this operation. |
If the run errors, cobra does not execute post runs. It is a somehow known issue (spf13/cobra#914) but problematic for Podmand as the runtime is shutdown during post run. Fixing the issue may fix a number of flakes. Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
If the run errors, cobra does not execute post runs. It is a somehow known issue (spf13/cobra#914) but problematic for Podmand as the runtime is shutdown during post run. Since some commands overwrite the post run and a general lack in cobra of post runs on errors, move the shutting down the engines directly into Execute. Fixing the issue may fix a number of flakes. Note that the shutdowns are NOPs for the remote client. Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
If the run errors, cobra does not execute post runs. It is a somehow known issue (spf13/cobra#914) but problematic for Podmand as the runtime is shutdown during post run. Since some commands overwrite the post run and a general lack in cobra of post runs on errors, move the shutting down the engines directly into Execute. Fixing the issue may fix a number of flakes. Note that the shutdowns are NOPs for the remote client. Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
I love the suggestion by @mislav on #914 (comment) but when you need to test the command with error having an |
If you don't need the If you need the command like me to prepare metrics of usage, you could store the in flight command and do process after the execute:
|
Reading the documentation on the front page, it is not clear to me what the best practice for raising errors from a command's
Run
func is?Looking at usage of cobra in other projects, the way seems to be just to return
os.Exit(1)
from the body of theRun
func. I'm a total Go noob so my instinct is probably wrong, but this didn't feel quite 'right' to me. Wouldn't that bypass anyPostRun
hooks? They feel like lifecycle events that should always get a chance to run though.Then I read to the bottom of issue #67 and I can see some code got merged four years ago adding a
RunE
hook which returnserror
. (Forgive me if I missed it but it seems this is not mentioned in the docs anywhere? https://github.com/spf13/cobra#prerun-and-postrun-hooks Are they deprecated?)This seemed like what I wanted, but the downside is this causes the 'usage' help text to be output. But that's not always appropriate - maybe the user gave perfectly good args to the command but somewhere in the processing of the job an error had to be returned.
So I have gone back to calling
os.Exit
from the body ofRun
. In the end this is fine for my use case.All this is just to say it would be good to have the framework's opinion on how to handle errors expressed in the docs somewhere.
The text was updated successfully, but these errors were encountered: