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

Don't disable flag parsing on the root command #600

Merged

Conversation

marckhouzam
Copy link
Contributor

The first commit of this PR is #594 for cobra 1.8.0

What this PR does / why we need it

This PR stops disabling flag parsing on the root CLI command to fix a small bug when updating to cobra 1.8.0.

I purposely kept this PR separate from #594 because even if we don't move to cobra 1.8.0, there is no reason for the CLI to disable flag parsing on its root command, as is explained below.

By disabling flag parsing on the root command, we tell Cobra not to parse flags when running the root command. Since there are are currently no flags on the root command this is not a problem. Furthermore, even with say, a global flag on the root command, this situation would still not be a problem because the root command is not actually runnable itself (no Run function), so flags would not be executed on the root command itself, but instead the DisableFlagParsing field would be read from the executable sub-command used.

However, there is an issue with not parsing flags on the root which is that it prevents Cobra's shell completion logic to complete the --help/-h flags on the root command. This is because all flag name completion becomes the responsibility of the program itself, and the program did not itself define --help/-h. This happens starting with Cobra v1.8.0.

We can see this by running tanzu __complete - and noticing the --help/-h flags are not suggested when using cobra 1.8.0.

The reason flag parsing was disabled on the root seems to be to allow plugins to handle their own flags. However, this is already done by each individual plugin sub-command created by GetCmdForPlugin() which does disable flag parsing already. There is therefore no reason to disable flag parsing on the root command.

Which issue(s) this PR fixes

Fixes # N/A

Describe testing done for PR

# Current behaviour of shell completion for the help flag on the root command
# This is correct, since the --help/-h flag is valid on the root command
$ git checkout main
$ make build
build darwin-arm64 CLI with version: v1.2.0-dev
mkdir -p bin
cp /Users/kmarc/git/tanzu-cli/artifacts/darwin/arm64/cli/core/v1.2.0-dev/tanzu-cli-darwin_arm64 ./bin/tanzu
$ tz __complete -
--help	help for tanzu
-h	help for tanzu
:4
Completion ended with directive: ShellCompDirectiveNoFileComp

# New behaviour of shell completion once we move to cobra 1.8.0
# This is now wrong as the --help/-h are not longer completed
$ git checkout feat/cobra1.8.0
Switched to branch 'feat/cobra1.8.0'
$ make build
build darwin-arm64 CLI with version: v1.2.0-dev
mkdir -p bin
cp /Users/kmarc/git/tanzu-cli/artifacts/darwin/arm64/cli/core/v1.2.0-dev/tanzu-cli-darwin_arm64 ./bin/tanzu
$ tz __complete -
:4
Completion ended with directive: ShellCompDirectiveNoFileComp

# Fix for this issue
$ git checkout fix/compDoubleHelpFlagForPlugins
Switched to branch 'fix/compDoubleHelpFlagForPlugins'
$ make build
build darwin-arm64 CLI with version: v1.2.0-dev
mkdir -p bin
cp /Users/kmarc/git/tanzu-cli/artifacts/darwin/arm64/cli/core/v1.2.0-dev/tanzu-cli-darwin_arm64 ./bin/tanzu
$ tz __complete -
--help	help for tanzu
-h	help for tanzu
:4
Completion ended with directive: ShellCompDirectiveNoFileComp

Release note

Don't disable flag parsing on the root command.

Additional information

Special notes for your reviewer

@marckhouzam marckhouzam requested a review from a team as a code owner November 20, 2023 19:08
@marckhouzam marckhouzam added this to the v1.2.0 milestone Nov 21, 2023
Copy link
Contributor

@anujc25 anujc25 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for explaining all the details as part of the description along with testing information.

By disabling flag parsing on the root command, we tell Cobra not to
parse flags when running the root command.  Since there are are
currently no flags on the root command this is not a problem.
Furthermore, even with say, a global flag on the root command, this
situation would still not be a problem because the root command is not
actually Runnable itself, so flags would not be executed on the root
command itself, but instead the DisableFlagParsing field would be read
from the executable sub-command used.

However, there is an issue with not parsing flags on the root which is
that it prevents Cobra's shell completion logic to complete the
--help/-h flags on the root command.  This is because all flag name
completion becomes the responsibility of the program itself, and the
program did not itself define --help/-h.  This happens starting with
Cobra v1.8.0.

We can see this by running `tanzu __complete -` and noticing the
--help/-h flags are not suggested when using cobra 1.8.0.

The reason flag parsing was disabled on the root seems to be to allow
plugins to handle their own flags.  However, this is already done by each
individual plugin sub-command created by `GetCmdForPlugin()` which does
disable flag parsing already.  There is therefore no reason to disable
flag parsing on the root command.

Signed-off-by: Marc Khouzam <kmarc@vmware.com>
@marckhouzam marckhouzam force-pushed the fix/compDoubleHelpFlagForPlugins branch from e34a263 to b5ff883 Compare November 21, 2023 19:50
@marckhouzam marckhouzam merged commit e470ecd into vmware-tanzu:main Nov 21, 2023
0 of 7 checks passed
@marckhouzam marckhouzam deleted the fix/compDoubleHelpFlagForPlugins branch November 21, 2023 19:50
@marckhouzam marckhouzam added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process label Dec 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-not-required kind/cleanup Categorizes issue or PR as related to cleaning up code, process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants