Use Cobra built-in mutually exclusive functionality

[jdknives]

Feature description

We currently manually detect mutually exclusive flags inside Skywire. Apparently there is a built-in function for that in Cobra, which we could use.

Documentation on the function https://pkg.go.dev/github.com/spf13/cobra#Command.MarkFlagsMutuallyExclusive

Example of case where we detect incompatibility manually:

https://github.com/skycoin/skywire/blob/222be5116e55f61211d57c440f776f4bd16dc6b0/cmd/skywire-cli/commands/config/gen.go#L183-L189

[Galzzly]

In order to make use of the MarkFlagsMutuallyExclusive functionality in Cobra you will need to update the package to at least version 1.5.0, as the function does not exist prior to that. Currently, Skywire is using version 1.4.0.

[jdknives]

Thanks for the hint. Will investigate that once I tackle this issue.

[Galzzly]

No problem. It's a pretty easy fix to implement once that is in place.

I do have the updated few files to resolve the issue, however, I was hitting problems with the make format check due to dependencies (reported via depguard), and didn't want to start pushing broader issues just to resolve this. I'm happy to share the updated files, though.

[0pcom]

The error message is not customizable with MarkFlagsMutuallyExclusive and also it cannot be made to use skywire-cli's logging

here is our current error:

$ go run cmd/*/*cli.go config gen -rf
[2023-06-21T19:06:32.31980946-05:00] FATAL [skywire-cli]: Use of mutually exclusive flags: -f --force cannot override -r --regen

And here is the more verbose yet more cryptic error when using the built-in MarkFlagsMutuallyExclusive

$ go run cmd/*/*cli.go config gen -rf
2023/06/21 19:05:04 Failed to execute command: if any flags in the group [regen force] are set none of the others can be; [force regen] were all set
exit status 1

[Galzzly]

So based on the above, you may need to look at setting the flagErrorFunc for the command.

In your var genConfigCmd section, add in

flagErrorFunc: func(_ *Command, err error) error {
    if (isForce) && (isRegen) {
        return fmt.Errorf("[%s] FATAL [skywire-cli]: Use of mutually exclusive flags: -f --force cannot override -r --regen", time.Now().Format("2006-01-02T15:04:05.999999999Z07:00"))
    }
    // these flags overwrite each other
    if (isUsrEnv) && (isPkgEnv) {
        return fmt.Errorf("[%s] FATAL [skywire-cli]: Use of mutually exclusive flags: -u --user and -p --pkg", time.Now().Format("2006-01-02T15:04:05.999999999Z07:00"))
    }
    return err
}

This is a very rough idea, and will need more ideal refinement. Without tracing back through your logger package to see if that can be made use of more than simply returning the formatted error, this was the simplest idea I could come up with.

From what I am able to ascertain from the cobra documentation, there is no specific function that will handle the error message for mutually exclusive flags being used, and I'm not particularly confident that the above will work as intended.

[jdknives]

Frankly, I dont think the default error message from Cobra is terrible. It surely is not bad enough that I would implement any specific error logging for it.

[Galzzly]

Indeed, however for consistency it would be nice to customise it in a way that you would like. Even if it's something so simple as to update the date format & app prefix as you have done for your logger implementation.

[0pcom]

I agree with @Galzzly ; and actually I didn't see an issue with the code without using MarkFlagsMutuallyExclusive

@jdknives if you think it's fine as-is; you can go ahead and review / merge #1629

@Galzzly i invited you to my fork of skywire ; if you can think of a way to integrate the logging feel free to push to the branch of my PR if you want to.

[Galzzly]

@0pcom - I'm taking a look at this now. I didn't get a chance over the weekend or yesterday.

[Galzzly]

I've gone through the Cobra code to try to understand where the log output for mutually exclusive flags can be set.

In short, outside of what you do already or the default mutually exclusive message, there currently does not appear to be a way to do this.

As a part of the execution of a command, the flags will be parsed and an error returned (which can be customised via the cmd.SetFlagErrorFunc function) - however, at this point, the flags are valid due to the exclusivity not being checked - then the help and version flags are checked, and then the PreRun (which is where you were performing your own exclusivity check). After the PreRun, the ValidateArgs is checked, and it is here where Cobra will error out and log about the mutually exclusive flags, and does not make use of the SetFlagErrorFunc, nor do I see a way to customise this output.

I will be raising an issue against Cobra for this, as I believe that this would be a useful addition to have.

With this in mind, though, for the time being - if you prefer to have the output per your logger, then it may be more worthwhile to keep your existing code as is unless you are happy with the default output from Cobra.

Apologies that I was not able to help too much with this.