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

Update Cobra to v1.5.0 #747

Merged
merged 2 commits into from
Jun 21, 2022
Merged

Update Cobra to v1.5.0 #747

merged 2 commits into from
Jun 21, 2022

Conversation

rolinh
Copy link
Member

@rolinh rolinh commented Jun 21, 2022

Cobra v1.5.0 is now released and with it comes a new API called flag groups that allows to mark flags as mutually exclusive or required together.

This PR gives it a go but other than saving a couple of lines of code, I'm not convince by the change from a user perspective.

Current output:

$ hubble observe --first 42 --all
cannot set both --first and --all

With this patch:

$ hubble observe --first 42 --all
if any flags in the group [first all] are set none of the others can be; [all first] were all set

Note that this new Cobra release also brings another new feature called active help. I'm not yet sure how it'd be best used in the context of the Hubble CLI.

EDIT: I ended up not opting for the new flag groups feature and removed the commit.

See release notes: https://github.com/spf13/cobra/releases/tag/v1.5.0

Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
@rolinh rolinh added ⌨️ area/cli Impacts the command line interface of any command in the repository. release-note/misc This PR makes changes that have no direct user impact. labels Jun 21, 2022
@rolinh rolinh requested a review from a team as a code owner June 21, 2022 06:27
@rolinh rolinh requested review from a team and tklauser and removed request for a team June 21, 2022 06:27
@rolinh rolinh changed the title Update Cobra to v1.5.0 and use new flags features Update Cobra to v1.5.0 and use new group flags feature Jun 21, 2022
@kaworu
Copy link
Member

kaworu commented Jun 21, 2022

$ hubble observe --first 42 --all
if any flags in the group [first all] are set none of the others can be; [all first] were all set

I find this error message more confusing than before the patch, unfortunately it cannot be customized: https://github.com/spf13/cobra/blob/5f2ec3c897155c3346e77430932e3966e5cfa648/flag_groups.go#L160. Let's see what other people think.

Also side question: aren't --all and --last also mutually exclusive?

@tklauser
Copy link
Member

$ hubble observe --first 42 --all
if any flags in the group [first all] are set none of the others can be; [all first] were all set

I find this error message more confusing than before the patch, unfortunately it cannot be customized: https://github.com/spf13/cobra/blob/5f2ec3c897155c3346e77430932e3966e5cfa648/flag_groups.go#L160. Let's see what the other people think.

I agree with Alex that the previous error message is more distinct and easier to read. As long as the message cannot be customized, I think we should stick to the old way of checking mutually exclusive flags.

@rolinh
Copy link
Member Author

rolinh commented Jun 21, 2022

Also side question: aren't --all and --last also mutually exclusive?

Indeed. I added a commit to handle this case.

As hinted in the PR description, I also agree that the new error message is not very clear nor succinct. I removed the commit that switched to using new flag groups.

Suggested-by: Alexandre Perrin <alex@isovalent.com>
Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 21, 2022
@tklauser tklauser merged commit 29b8258 into master Jun 21, 2022
@tklauser tklauser deleted the pr/rolinh/cobra-1.5 branch June 21, 2022 11:31
@rolinh rolinh changed the title Update Cobra to v1.5.0 and use new group flags feature Update Cobra to v1.5.0 Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⌨️ area/cli Impacts the command line interface of any command in the repository. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants