Use FlagErrorFunc for validateRequiredFlags and validateFlagGroups

[SVilgelm]

This let's handle all flag errors and decide how to print them on the fly

[CLAassistant]

All committers have signed the CLA.

[marckhouzam]

Thanks @SVilgelm ! On first look this does makes sense. However I am concerned about backward-compatibility where an existing program would unexpectedly see it's FlagErrorFunc() being called for a new purpose.

What we could do is introduce a new ValidateFlagErrorFunc to be used in the two flag validation cases and keep the existing FlagErrorFunc() just for the flag parsing error case.

I wonder what @jpmcb and @johnSchnake think about this?

[SVilgelm]

introduce a new ValidateFlagErrorFunc

yeah, make sense. I think about adding two more RequiredFlagsErrorFunc and FlagGroupsErrorFunc functions, each per case. But on another hand the validateFlagGroups is not released yet, so we can easily modify it and it means that we have the backward compatibility issue with the validateRequiredFlags function only.

WDYT?

[marckhouzam]

I agree with the flag group one since it's not released 👍

I wonder if we're introducing too much complexity? On the other hand, backwards-compatibility is super important in this project so I don't think we have a choice.

Can you give an example of what you'd like to do with this proposed new functionality @SVilgelm ?

[SVilgelm]

I pushed as second commit here, will squash if it is ok

[SVilgelm]

the first example is controlling how to print an error, we use zerolog and I want to print any our error in zero log format (as json), but any cobra error should be printed in the standard format, because it is misconfiguration issue and should not be collected and send to observe

[SVilgelm]

something like this:

type cobraError struct {
	err error
}

func (e cobraError) Error() string {
	return e.err.Error()
}

func (e cobraError) Is(err error) bool {
	var r cobraError
	return ferrors.As(err, &r)
}

func wrapError(_ *cobra.Command, err error) error {
	return cobraError{err}
}

// Execute runs given command and properly handle returned error
func Execute(cmd *cobra.Command) {
	cmd.Use = path.Base(os.Args[0]) + " [OPTIONS] [arg...]"
	// disable printing error and usage by cobra
	cmd.SilenceUsage = true
	cmd.SilenceErrors = true
	cmd.SetFlagErrorFunc(wrapError)

	if err := cmd.Execute(); err != nil {
		if ferrors.Is(err, RequiredFlagErr{}) || ferrors.Is(err, cobraError{}) {
			cmd.PrintErrln("Error:", err)
			cmd.Usage()
		} else {
			// logs fatal error if no cobra error
			log.Fatal(cmd.Context()).Err(err).Send()
		}
		os.Exit(1)
	}
}

[SVilgelm]

@marckhouzam any thoughts about the changes?

[github-actions[bot]]

The Cobra project currently lacks enough contributors to adequately respond to all PRs. This bot triages issues and PRs according to the following rules:

• After 60d of inactivity, lifecycle/stale is applied. - After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied and the PR is closed. You can:
• Make a comment to remove the stale label and show your support. The 60 days reset. - If a PR has lifecycle/rotten and is closed, comment and ask maintainers if they'd be interseted in reopening.

[SVilgelm]

@marckhouzam @jpmcb could you please find a time and review this PR?