Fix handling of errorShouldDisplayUsage#2469
Conversation
- Exit with a non-zero exit code - Actually report the error we detected - Write usage to stderr Signed-off-by: Miloslav Trmač <mitr@redhat.com>
c132ef7 to
27baed9
Compare
|
|
||
| // errorShouldDisplayUsage is a subtype of error used by command handlers to indicate that cli.ShowSubcommandHelp should be called. | ||
| // errorShouldDisplayUsage is a subtype of error used by command handlers to indicate that the command’s help should be included. | ||
| type errorShouldDisplayUsage struct { |
There was a problem hiding this comment.
Is this a cobra idiom? Is there equivalent to this in other projects we maintain?
Basically just wondering if this could/should be abstracted/reused in multiple places to make bugs like this less likely.
There was a problem hiding this comment.
Is this a cobra idiom?
Not… particularly? cobra has a default behavior of printing usage on every error. That seems excessive, but anyway Skopeo opts out (via SilenceUsage), and so do the other projects.
Cobra’s idiomatic way to handle these particular checks is via command.Args (e.g. Args: cobra.ExactArgs(2)). That doesn’t do anything special about printing, or not printing, the usage message.
Skopeo isn’t even consistent internally in usage of this errorShouldDisplayUsage; it happened to be created when migrating to Cobra, and it was sporadically copy&pasted into some of the other commands.
Printing the usage message here comes from way back, f398c9c , with no documented rationale or reference to any specific convention (e.g. in particular the GNU coding standards are silent on this).
I don’t feel particularly strongly about this behavior; if I were told to just report the error and drop all this usage logic, I wouldn’t object. As for consistently printing the usage documentation in other subcommands, I’m not opposed but I’m also not going to work on that now, given other priorities — it’s just the current behavior, returning a success exit code, that I think is unacceptable with high priority.
WRT reuse in other projects, not easily: the commandAction wrapper exists (due to some … lessons learned :) ) to make the Cobra API unavailable in the command handlers, to force everything to go through the option structs and never look up flags by string names. I do think that’s a better design, but using cobra flag lookups by name is extremely widespread throughout Buildah/Podman (in some cases it’s the only way the flags get shared between the Buildah and Podman codebases), so commandAction can’t be practically adopted by Buildah/Podman right now.
Fixes #2468.