Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MarkFlagsRequiredTogether function does not work as expected if any required flag provided #1843

Closed
bilalcaliskan opened this issue Oct 28, 2022 · 4 comments

Comments

@bilalcaliskan
Copy link

Hello folks, I have noticed a strange behavior about MarkFlagsRequiredTogether function. I am using cobra v1.6.1 with Golang 1.19.1. Here is the code snippet:

rootCmd.MarkFlagsRequiredTogether("accessKey", "secretKey", "bucketName", "substring", "region")

If i dont provide any of the flags, i expect cobra to throw an error but somehow it is not throwing an error. It passes that line and fails on normal application flow as expected.

BUT; if i provide at least one required flag, it gives me my desired error like that(assume that i have provided secretKey flag:

Error: if any flags in the group [accessKey secretKey bucketName substring region] are set they must all be set; missing [accessKey bucketName region substring]

Btw i am willing to contribute that project by finding the problematic part of the code and throwing desired error. Cheers!

@Luap99
Copy link
Contributor

Luap99 commented Oct 28, 2022

That is the point of MarkFlagsRequiredTogether() as it is described in that function comment. Looks like you want MarkFlagRequired().

@bilalcaliskan
Copy link
Author

bilalcaliskan commented Oct 29, 2022

I dont get it, i think in my case cobra should also throw an error. Do you mean that i must call MarkFlagRequired() for each required field instead of MarkFlagsRequiredTogether? Function description specifies the behavior of itself like that:

// MarkFlagsRequiredTogether marks the given flags with annotations so that Cobra errors
// if the command is invoked with a subset (but not all) of the given flags.

In that description, it says cobra will throw an error if i call i dont provide AT LEAST 1 required flag which is the function parameter. Am i missing something?

As a summary, if it is something like you said, i would definetely suggest you to change that function's name because with that name and the behavior of you mentioned, it is literally misunderstandable.

@Luap99
Copy link
Contributor

Luap99 commented Oct 29, 2022

I dont get it, i think in my case cobra should also throw an error. Do you mean that i must call MarkFlagRequired() for each required field instead of MarkFlagsRequiredTogether?

yes

// MarkFlagsRequiredTogether marks the given flags with annotations so that Cobra errors
// if the command is invoked with a subset (but not all) of the given flags.

In that description, it says cobra will throw an error if i call i dont provide AT LEAST 1 required flag which is the function parameter. Am i missing something?

It says: if the command is invoked with a subset (but not all) of the given flags I guess it is open to interpretation if no flags are considered a subset.

The name cannot be changed since it is part of the Public API which isstable. For me, MarkFlagsRequiredTogether means that they are required together (if I set one I have to set the others as well).
It is also described here: https://github.com/spf13/cobra/blob/10cf7be9972ee5b5994cf760ecba642309ac8685/user_guide.md#flag-groups

I am not a maintainer but you can always create a PR to improve the wording. PR #1654 implemented the feature for reference.

@bilalcaliskan
Copy link
Author

@Luap99 thanks for your detailed explanation, my bad!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants