Add ability to mark flags as required or exclusive as a group

[johnSchnake]

This change adds two features for dealing with flags:

• requiring flags be provided as a group (or not at all)
• requiring flags be mutually exclusive of each other

By utilizing the flag annotations we can mark which flag groups
a flag is a part of and during the parsing process we track which
ones we have seen or not.

A flag may be a part of multiple groups. The list of flags and the
type of group (required together or exclusive) make it a unique group.

Fixes #1654
Fixes #1595
Fixes #1238

Signed-off-by: John Schnake jschnake@vmware.com

[github-actions]

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

[johnSchnake]

When doing the review of the backlog this feature came up a few times and I have personally wanted to do this as well in my other projects so I thought I would put something up.

I utilize the flag annotations which up to this point had just been used for shell completions. This doesn't interact with the shell completions at all but is just validation logic. I don't know if that is a hard stop for someone, but I thought I'd put this up and see.

I'll have to lookup the issues that this would impact (don't have them handy right now) but wanted it put up for 👀 . There is surely some cleanup that could occur as well, feel free to comment. I just wanted to make sure it had functioning logic and tests to make it clear how it looked in its current state.

[github-actions]

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

[github-actions]

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

[johnSchnake]

The 200 line "suggested max" is exceeded but it includes tests and a new file (so a new file/license header). Regardless, I also think there is a bit of code I can compact.

[github-actions]

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

[johnSchnake]

Pulling some common code out caused some reduction in LOC but then continuing to extract logic into functions for easier reading caused some increase. Not worrying about the LOC unless others think it is a problem. I think the new arrangement of code is easier to understand.

[johnSchnake]

2 new annotations. No utilization for completion, but only for validation.

[marckhouzam]

I like that. I think it is just fine to use the annotations field of a flag for anything we need (even if there are pflag comments saying Annotations are used for completions...)

However, do we need them public? Based on the proposed solution, I don't expect users to access the annotation directly, right?

[johnSchnake]

Definitely open to verbage changes if someone wants to suggest anything. Pointing out the first [%v] I manually add the brackets since the flagList is just a string even though it is of the form flagA flagB flagC.

[marckhouzam]

The wording seems ok to me. I'm just a little unsure about the format when multiple errors are concatenated together. For example:

./prog child --automatic yes --destination a --fast-deploy fish --effective a                                                       <k8s-lab>
Error: if any flags in the group [automatic best certificate] are set they must all be set; missing [best certificate], if any flags in the group [destination fast-deploy] are set none of the others can be; [destination fast-deploy] were all set
Usage:
  prog child [flags]

Flags:
[..]

Should the error messages be separated by a newline character?

[johnSchnake]

A newline would work for me; I also didnt like the format but tried to follow the general Go guidance of not using capital letters or ending punctuation.

So you'd prefer something like :

Error: if any flags in the group [automatic best certificate] are set they must all be set; missing [best certificate]
if any flags in the group [destination fast-deploy] are set none of the others can be; [destination fast-deploy] were all set

Usage:...

I could also try and make it so that it makes it like a bulleted list? They are long errors so it doesnt flow particularly great if you have multiples:

Error: 
 - if any flags in the group [automatic best certificate] are set they must all be set; missing [best certificate]
 - if any flags in the group [destination fast-deploy] are set none of the others can be; [destination fast-deploy] were all set

Usage:...

I think the latter looks the best for and end user on the CLI and is just a tiny bit more verbose to implement. What do you think?

[Luap99]

Why do you have to return all errors? The normal flag parsing code only returns the first error it encounters. This should simplify the code and make the errors smaller and thus better readable.

[marckhouzam]

Why do you have to return all errors? The normal flag parsing code only returns the first error it encounters. This should simplify the code and make the errors smaller and thus better readable.

I guess it was to avoid a user having to fix one error at a time, if there were multiple errors?
I see value in both approaches personally, so I'll defer to what you guys settle on.

That being said, the current code is missing the first newline when there are multiple errors:

Error: - if any flags in the group [automatic best certificate] are set they must all be set; missing [best certificate]
- if any flags in the group [destination fast-deploy] are set none of the others can be; [destination fast-deploy] were all set

[johnSchnake]

I'm definitely fine with the simplification. Not sure why I defaulted to that originally, I sometimes use both. Making the simplification avoids that problem all together and since its just at invocation time and feedback is fast I dont think its a problem if they have to tweak it a few times and keep finding 'new' problems.

[marckhouzam]

Nice improvement @johnSchnake! I'm interested in having a more detailed look soon.
We will probably want to update the documentation to tell users how they can use the feature.

[marckhouzam]

What should we do about persistent flags for this feature? Can they be part of groups for example?

[johnSchnake]

Thanks for the call on persistent flags. I'll double check how they're handled. I know there were some other existing issues about them not getting merged in certain validations. My assumption should be merged prior to this and valid for use with this feature. I'll make sure tests cover that.

[github-actions]

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

[github-actions]

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

[github-actions]

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

[johnSchnake]

Updated docs so that didnt slow things down later if this gets 👍

[Luap99]

Not a proper review but we should try to add the mutually exclusive logic to the shell completion logic, i. e.
if I have mutually exclusive flags foo and bar the shell completion logic should no longer recommend --bar when --foo is already present on the cli.

[johnSchnake]

@Luap99 I have no idea regarding shell completions. Could that potentially go in a as a separate PR from someone who has context on that? That seems reasonable to me since it isn't obvious that devs with 1 piece of context will have the other and shell completion is just a convenience feature and not something necessary for the functionality.

I have no problem creating an issue to track it as I've pointed out that shortcoming myself too.

[marckhouzam]

This is pretty cool! I think it is a nice improvement for our users. Nice job @johnSchnake .
I've added some suggestions in-line for your consideration.

I'll have a look at the completion logic in the mean time.

[marckhouzam]

I like that. I think it is just fine to use the annotations field of a flag for anything we need (even if there are pflag comments saying Annotations are used for completions...)

However, do we need them public? Based on the proposed solution, I don't expect users to access the annotation directly, right?

[marckhouzam]

The wording seems ok to me. I'm just a little unsure about the format when multiple errors are concatenated together. For example:

./prog child --automatic yes --destination a --fast-deploy fish --effective a                                                       <k8s-lab>
Error: if any flags in the group [automatic best certificate] are set they must all be set; missing [best certificate], if any flags in the group [destination fast-deploy] are set none of the others can be; [destination fast-deploy] were all set
Usage:
  prog child [flags]

Flags:
[..]

Should the error messages be separated by a newline character?

[github-actions]

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

[Luap99]

@Luap99 I have no idea regarding shell completions. Could that potentially go in a as a separate PR from someone who has context on that? That seems reasonable to me since it isn't obvious that devs with 1 piece of context will have the other and shell completion is just a convenience feature and not something necessary for the functionality.

I have no problem creating an issue to track it as I've pointed out that shortcoming myself too.

Creating an issue to track this would be great. Maybe I can find some time to work on this.

[marckhouzam]

@Luap99 I have no idea regarding shell completions. Could that potentially go in a as a separate PR from someone who has context on that? That seems reasonable to me since it isn't obvious that devs with 1 piece of context will have the other and shell completion is just a convenience feature and not something necessary for the functionality.
I have no problem creating an issue to track it as I've pointed out that shortcoming myself too.

Creating an issue to track this would be great. Maybe I can find some time to work on this.

I coded something last night but I didn't know how to share it. Sorry @Luap99 .
I guess I can create a PR on top of this one. It's just that I don't like that it will contain the changes of this PR as well...
If there is a better way to do such things, I'd love to know.

[marckhouzam]

This is good for me once the linter and format errors are fixed.

[github-actions]

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

[johnSchnake]

All green; going to squash down to 1 commit

[github-actions]

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

[marckhouzam]

I was all ready to approve but I did one last testing pass and I hit something I hadn't noticed before.

Say I have a root command with a persistent --debug flag, and a child command with a --details flag. Then the child marks both flags as required together. I noticed that I can no longer call the root command with the --debug flag as it requires the --details flag, but that flag does not exist on the root.

Here's a test program:

package main

import (
	"fmt"

	"github.com/spf13/cobra"
)

var (
	debug   bool
	details bool
)

var rootCmd = &cobra.Command{
	Use: "prog",
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("rootCmd called")
	},
}

var logCmd = &cobra.Command{
	Use: "log",
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("child called")
	},
}

func main() {
	rootCmd.AddCommand(logCmd)

	rootCmd.PersistentFlags().BoolVar(&debug, "debug", false, "debug")
	logCmd.Flags().BoolVarP(&details, "details", "d", false, "details")

	logCmd.MarkFlagsRequiredTogether("details", "debug")

	rootCmd.Execute()
}

then I run this:

$ go build -o prog groupFlag.go
$ ./prog --debug
Error: if any flags in the group [details debug] are set they must all be set; missing [details]
[...]
# The root command wants the flag group although it was the child command that created it

$ ./prog --debug --details
Error: unknown flag: --details
[...]

It's hard for me to judge if this is a valid scenario, but it seems like it is plausible, no?

I haven't looked into the logic causing this.

[marckhouzam]

That last test I ran made me think of a situation that is not covered by the PR. Maybe it's not important and we simply won't support it, I just wanted to mention it to get your opinion.

Let's use an example to describe the use case. Say I have a --user flag and a --password flag used to login to a system. But say I am allowed to use the --user flag by itself, but not the --password flag. So, if the user types --password the --user flag is required but not vice versa.

Any thoughts on this @johnSchnake ?

[johnSchnake]

Let's use an example to describe the use case. Say I have a --user flag and a --password flag used to login to a system. But say I am allowed to use the --user flag by itself, but not the --password flag. So, if the user types --password the --user flag is required but not vice versa.

@marckhouzam that is an interesting use case. I do think thats a more narrow use-case so I don't mind leaving that off (users can still manually implement it as they would today); the only alternative would be to do one of two things it seems:

• redefine all of this in terms of per-flag triggers: if flag X is defined, then this separate list is required/exclusive. This would mean if a flag group has 3 elements you'd have to define 3 flag groups to reproduce the behavior we have in this. I think this is a much less intuitive and more verbose way to implement the feature for end users.
• Alternatively, you could keep this as-is and add the above functionality as a separate feature. In that case it wouldn't impact this PR necessarily and I think we should wait to see if people actually complain/request that specific feature. Otherwise we risk adding unnecessary features/functions into the library, clouding the more useful, requested ones.

[marckhouzam]

• Alternatively, you could keep this as-is and add the above functionality as a separate feature. In that case it wouldn't impact this PR necessarily and I think we should wait to see if people actually complain/request that specific feature. Otherwise we risk adding unnecessary features/functions into the library, clouding the more useful, requested ones.

Sounds like a good approach to me.

[johnSchnake]

Is this still good to go or did I miss a separate concern?

[marckhouzam]

I think we have an inconsistency when dealing with persistent flags. I tried to describe it in the comment above: #1654 (review)

Maybe we made thing overly complex by supporting persistent flags? If the feature does not really need to support them, I'd be ok with documenting that persistent flags should not be part of groups. It depends on how you imagine the feature being use @johnSchnake.

[johnSchnake]

My thoughts on this:

• when marking the flags as a part of a group, we ensure the flags do exist in the context they're being defined (e.g. on the logCmd in your example); so erroring like the code did is the worst outcome
• when actually checking flag groups we start with, potentially, another command (e.g. rootCmd) where only a subset of those flags may exist (the persistent one). Either the flag should be ignored or the entire flag group.

I lean towards modifying the code so that the entire flag group is ignored if not every flag is found. That seems to be the easiest mental model to understand, is easily written into the documentation, and still facilitates the the use case you showed.

Documentation will just be updated to reflect something like:

If persistent and local flags are in flag groups together, the group will not be enforced on commands which do not have all the relevant flags defined.

[github-actions]

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

[marckhouzam]

Nice!
LGTM

@jpmcb are you ok with this new feature? If we don't hear from @jpmcb by Friday afternoon, and since both @johnSchnake and I approved, I suggest @johnSchnake go ahead and merge.

[johnSchnake]

Just squashing preemptively. No other changes on this next push, just FYI

[github-actions]

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

[Dentrax]

Also fixes #1385