Remove duplicate messages and unwanted usage messages when an Mixer error happens

[cmarcelo]

No description provided.

[cmarcelo]

@tmarcu @kevincwells could you please take a look? I'm not familiar with Cobra to know if there is a better way to do it.

I've looked at spf13/cobra#340 to reach this solution.

[tmarcu]

So it will not output the usage when the command entered is actually wrong though?

[cmarcelo]

Yes.

$ mixer build --ha
Mixer 3.2.1
Mixer Error: unknown flag: --ha

I couldn't find a Cobra way to avoid the spam and still print the usage messages when it failed to parse stuff (where would be reasonable to show the usage). Note that calling "mixer build", "mixer build -h" or "mixer build --help" will show the usage message.

[kevincwells]

So just to be a little more precise, the two separate issues are:

1. You get a usage message even when you "use" the command correctly, but it dies for some other reason.
2. You're getting duplicate error messages printed.

Regarding 1), I'm a bit torn, but overall I lean in the other direction. I don't like the idea of getting a usage string when you've technically "used" it right, but I like even less the idea of not getting one when you have messed up. Detecting malformed usage and reporting it back is one of the major usability niceties of a library like Cobra. (For the record, I checked, and suppressing usage like this doesn't affect subcommand "did you mean" suggestions, so that's good.)

As for 2) We're getting duplicate messages because we're both letting Cobra's default error handling behavior work and manually printing the error message that's rippled up through RunE. We need to do one or the other. Your patch turns off the automatic error output, and lets the manual top-level error reporting print it out. An alternative would be to just remove line 61 (fmt.Fprintf(os.Stderr, "Mixer Error: %s\n", err)) from root.go and leave the automatic error handling alone.

[cmarcelo]

Detecting malformed usage and reporting it back is one of the major usability niceties of a library like Cobra.

Note that it will still do that (see the reply to tudor above), what it won't do is automatically show the entire usage help in case of such failure.

I'll take another pass to see if we can improve things. Thanks for pointing out the Execute function, I was just ignoring it assuming it was auto generated and doing the right thing.

[kevincwells]

Note that it will still do that (see the reply to tudor above), what it won't do is automatically show the entire usage help in case of such failure.

Fair point. It does tell you what you've done wrong, it just doesn't tell you how to do it right. I think I am fine with that state as a compromise for avoiding the usage message when you technically used it right.

What I'd want is a more fine-grained control that Cobra doesn't (yet) offer: the ability to show usage messages on usage errors, but not on other errors. Even then, deciding to use such a feature would be a decision on what percentage of the time you think people know what they're doing and just typo'd, vs don't know what they're doing and would immediately follow up such an error with a "mixer -h" to see the usage. But it'd at least be nice to have the option.

[cmarcelo]

@kevincwells could you take another look?

[cmarcelo]

So it will not output the usage when the command entered is actually wrong though?

Changed the patch, so now it WILL show usage information when the command is wrong. (A bit of a hack going on there, but seems good enough).

[tmarcu]

Parsing and matching error strings seems so wrong :(

[cmarcelo]

I've added an comment above explaining why we can't do the nice way. If pflags returned errors from a specific type, we could switch on type here and do a cleaner job, but they don't.

cobra controls the parsing of the flags and when it happens, so we can't really do much from the outside. One alternative would be looking at replacing the UsageFunc for the commands and print from there or store the information. Would have to dig deeper in how cobra uses it and I didn't think was worthwhile.

Given that "failing" here is not critical (i.e. we either miss showing a Usage message or show one extra Usage message), I guess we should be fine.

That said, I'm also happy to take this out and just use the approach from the first patch. I just don't want SPAM on every error :-)

[kevincwells]

I really like the goal of this, but I agree with @tmarcu that matching on error string content just feels too hacky and fragile. You're absolutely right about switching on error types -- I think having the ability to discern parsing errors from others would be a useful enough feature that I think I might actually go submit an Issue on their pflag library asking for it.

I think I'd rather lose the automatic usage output altogether until a more reliable solution is possible.

[kevincwells]

Oooo, I found it! There's a method called SetFlagErrorFunc. By default, its companion FlagErrorFunc is a noop, returning a function that just returns the original error. But you can override this to do whatever you want... like print the usage!

In root.go's init(), insert this:

	RootCmd.SetFlagErrorFunc(func(cmd *cobra.Command, err error) error {
		cmd.Print(cmd.UsageString())
		return err
	})

Now usage is supressed for all errors thanks to SilenceUsage, but if a flag error occurs, it prints the ussage and then passes it back up. Produces output like:

root@machine /home/clr/mix # mixer add-rpms -t
Mixer 3.2.1
Usage:
  mixer add-rpms [flags]

Flags:
  -c, --config string   Builder config to use
  -h, --help            help for add-rpms
Mixer Error: unknown shorthand flag: 't' in -t

[cmarcelo]

Great! I guess I should’ve keep digging. I’ll update the PR and test tomorrow.

[kevincwells]

Fair warning: this doesn't trigger when other types of "usage" errors occur -- in other words, usages caught by Cobra itself, rather than pflag.

For example, if you type mixer foobar -- a command usage error, not a flag usage error -- you still get the error message with no usage info attached. Similarly, argument errors are not caught with this, so if we took advantage of Cobra's "this command must have between 2 and 4 arguments in addition to any flags" type checks, that wouldn't spit usage either. (Potentially that could be caught with use of the ValidateArgs function?)

I'm not too worried about these other cases, and the above flag error function covers a lot. And as @cmarcelo already pointed out, it's not like the malformed usage isn't being caught, it's just not resulting in a usage message.

[kevincwells]

@tmarcu and others -- what are your thoughts on SilenceErrors vs not re-printing errors in the Root's Excute()?

At a higher level, what is the goal of passing errors back up the tree and printing them at the root? Asked a different way, what types of situations are we trying to catch that won't get printed via the automatic error output that SilenceErrors suppresses?

[cmarcelo]

Pushed a new version earlier today that don't use SilenceXXX. It simply don't pass mixer errors back to cobra, applying them directly (i.e. use Run instead of RunE).

[kevincwells]

👍 LGTM. Basic testing behaved as I expected.

@tmarcu -- this came out of a discussion @cmarcelo and I had about the many different ways to solve the basic problem of distinguishing mixer internal errors form mixer CLI errors.

We settled on passing helper errors back up to builder, and builder errors back up to the CLI Run functions that called them, but handling them there. That way, we can use whatever mechanism we want to handle internal errors (the new fail() function for now, or whatever logging/reporting solution we want in the future), and the only errors Cobra is trying to handle are those related to Cobra.

This separation has the benefit of no longer needing SilenceErrors and SilenceUsage -- it basically "does the right thing" now: mixer internal errors get reported as-is, Cobra errors get usage feedback.

Do you have any concerns with this approach, or did you have a strong reason for using RunE and relying on Cobra's error reporting originally?

[tmarcu]

Doing it with a fail() kind of function was an option as well, and I'm not opposed to it. I can't give you a strong enough reason for using RunE for mixer specifically, and I don't see it becoming some library either, so I think as long as we exit with an error code that's enough. I wasn't a fan of SilenceErrors/Usage, so this is +1 IMO.
Also @cmarcelo do you mind rebasing, I think one of the other PR merges broke this :)

[cmarcelo]

@tmarcu rebased.