Skip flag completion for commands with disabled flag parsing

[scothis]

When a command disabled flag parsing, the shell completion for that
command should not offer flags. This allows completions for args to be
processed like normal. This behavior is particularly useful for invoking
a CLI plugin that has its own sub commands, arguments, flags, and
completion rules.

Signed-off-by: Scott Andrews andrewssc@vmware.com

[jpmcb]

cc @marckhouzam - would be great to get your opinion on this

[marckhouzam]

Thanks @scothis. There is already some special handling for the DisableFlagParsing option in the completion code but it may not cover every case.

Could you provide an example scenario to better illustrate the case you refer to?

[scothis]

The basic problem I was hitting is that cobra either wants to provide completions for commands/args or completions for flags. In a CLI I've been contributing to (sorry it's not public) has a plugin model where plugins are mounted into the root command as child commands that then exec the plugin passing the remaining args. These commands are using ValidArgsFunction to proxy the completion into to the plugin's completion command. This approach is working well for command, argument and flag-value completion, but fails for flag-name completion because the ValidArgsFunction isn't called on the root CLI's plugin command. These plugin commands have DisableFlagParsing set to avoid the flags defined on the plugin from being parsed.

This PR is attempting to make cobra completion also ignore flags when DisableFlagParsing, while still allowing arg completion.

[marckhouzam]

I'm glad someone else needs this fix @scothis. This is the same problem as what #1161 is trying to fix.
Helm uses a similar mechanism for its plugins as what you describe and I hit the same limitation.

However, after experimenting with the fix, #1161 chooses to still do flag completion when DisableFlagParsing==true, and then continues on to ValidArgsFunction (see #1161 (comment)). This allows the completion system to complete persistent flags of the root command even for plugins. For example, helm has a --namespace global flag so doing

$ helm myplugin --nam<TAB>

still performs the completion for --namespace without having to have the plugin handle it.

Would that be a scenario valuable for you as well?

Note that #1161 seems to have a big diff, but it is mostly just indentation changes, the actual code changes are quite small.

[scothis]

@marckhouzam good to see someone else thinking along similar lines. I wonder if cobra should take any responsibility for persistent flags when the command has asked for flag parsing to be disabled. This is something that could be addressed in user space by the custom completion for the command taking flags into consideration.

As a meta question, I'm curious as to why cobra draws such a strong distinction between completions for commands/args and flags.

[marckhouzam]

@marckhouzam good to see someone else thinking along similar lines. I wonder if cobra should take any responsibility for persistent flags when the command has asked for flag parsing to be disabled. This is something that could be addressed in user space by the custom completion for the command taking flags into consideration.

My thought was that it simplifies the responsibility of the plugin developer. Also, if we don't do that and let plugins deal with completion of global flag names, then, when a new global flag is added, every plugin will have to be updated.

Do you worry about a scenario in particular?

As a meta question, I'm curious as to why cobra draws such a strong distinction between completions for commands/args and flags.

It doesn't really, it's just that completion of args and commands always trigger ValidArgsFunction so it works out of the box for the plugin scenario. Flags names never trigger ValidArgsFunction as it didn't make sense until this scenario came up.

[scothis]

It doesn't really, it's just that completion of args and commands always trigger ValidArgsFunction so it works out of the box for the plugin scenario. Flags names never trigger ValidArgsFunction as it didn't make sense until this scenario came up.

Another way to solve the issue would be to allow flags and args to complete simultaniously. The root cause of the issue that drove me to open this PR was that when flag completion triggered, there was no opportunity to provide custom arg completion. Something that could be a bit sticky is handling conflicting ShellCompDirectives.

My thought was that it simplifies the responsibility of the plugin developer. Also, if we don't do that and let plugins deal with completion of global flag names, then, when a new global flag is added, every plugin will have to be updated.

I assume you'd add this behavior once in the command that is hosting the plugin, and not in every plugin. When adding a new persistent flag, don't you already have the issue of how to pass that to the plugin? Our plugin model doesn't have any persistent flags from the root command, so we've side stepped this issue so far.

[marckhouzam]

Another way to solve the issue would be to allow flags and args to complete simultaniously.

IIUC, this would imply that when Cobra does flag name completion, it would also call ValidArgsFunction, like how we do things for subcommands. I think it would have been a possible approach, but it would add complexity for developers that would then need to consciously handle the case of flag names in their ValidArgsFunction code; the benefit however would only be in the case where DisableFlagParsing==true, so we might as well stick to that much more specific scenario.

Also, I think changing this behaviour now would cause some backwards-compatibility issues for users of Cobra.

My thought was that it simplifies the responsibility of the plugin developer. Also, if we don't do that and let plugins deal with completion of global flag names, then, when a new global flag is added, every plugin will have to be updated.

I assume you'd add this behavior once in the command that is hosting the plugin, and not in every plugin. When adding a new persistent flag, don't you already have the issue of how to pass that to the plugin?

You are probably right about that.

There is another scenario where flag-name completion is required for the Helm plugin system (which I tried to avoid getting into, as it is harder to explain). Basically helm allows a plugin to provide a completion.yaml file to specify the flag names that are valid for the plugin (as well as validArgs and sub-commands) (https://helm.sh/docs/topics/plugins/#static-auto-completion). When reading that file, helm adds these flags to Cobra with the goal of Cobra handle the flag-name completion automatically (same for validArgs and sub-commands of the plugin). This, I felt, was a simple approach for plugin developers (a yaml file), instead of forcing them to handle the completion in a script for example.

In this solution, Cobra knows about flag-names, but the plugin sub-command still sets DisableFlagParsing==true. So #1161 supports this case of doing flag-name completion automatically before calling ValidArgsFunction.

Did you try using #1161 with your tool to confirm that it would satisfy your needs?

[scothis]

@marckhouzam thanks for the reminder. #1161 does also solve our issue. I only tested with the __complete command and didn't regenerate the shell completion script

[marckhouzam]

Thanks for confirming @scothis. Therefore, in my humble opinion, #1161 provides a little more flexibility and it also addresses helm's advanced plugin use cases.

So @jpmcb I'd recommend going with #1161 if you agree.

[jpmcb]

Let's stick with #1161 since it will provide a bit more flexibility. Thanks for the great conversation on this one everyone!