Unit tests claim --version flag should work on sub command, but it does not

[brianpursley]

The user guide says that the automatic --version flag is a top-level flag on the root command:

Cobra adds a top-level '--version' flag if the Version field is set on the root command. Running an application with the '--version' flag will print the version to stdout using the version template. The template can be customized using the cmd.SetVersionTemplate(s string) function.

This seems to match what it really is doing, because I can see the --version flag is not added as a persistent flag, so I would not expect it to work on sub commands.

However, a couple of unit tests contradict this, saying that it should work on sub commands: https://github.com/spf13/cobra/blob/ad6db7f8f6e485f55b1561df9276fa4d8e278bde/command_test.go#L1102-L1112 https://github.com/spf13/cobra/blob/ad6db7f8f6e485f55b1561df9276fa4d8e278bde/command_test.go#L1114-L1124

While these tests pass, it is only coincidental, because the --version flag does not actually work for sub commands, as illustrated by this test program and the following examples:

package main

import (
	"github.com/spf13/cobra"
)

func main() {
	emptyRun := func(*cobra.Command, []string) {}
	rootCmd := &cobra.Command{Use: "root", Version: "1.0.0", Run: emptyRun}
	rootCmd.AddCommand(&cobra.Command{Use: "sub", Run: emptyRun})
	_ = rootCmd.Execute()
}

Example 1: Base case, works as expected for the root command

$ go run mycmd.go --version
root version 1.0.0

Example 2: Pass sub command after the flag. This is what the unit test does.

$ go run mycmd.go --version sub
root version 1.0.0

Example 3: Pass sub command before the flag. Now you can see it's not really working for sub commands.

$ go run mycmd.go sub --version
Error: unknown flag: --version
Usage:
  root sub [flags]

Flags:
  -h, --help   help for sub

Example 4: Pass an argument to the sub command. This shows that cobra must be interpreting "sub" as a flag value because it considers foo to be the sub command

$ go run mycmd.go --version sub foo
Error: unknown command "foo" for "root"
Run 'root --help' for usage.

Example 5: To further show that it is skipping over the sub, I can pass sub twice and it then says --version is an unknown flag for sub:

$ go run mycmd.go --version sub sub
Error: unknown flag: --version
Usage:
  root sub [flags]

Flags:
  -h, --help   help for sub

I'm thinking we should just get rid of these two unit tests.

My motive for requesting to delete these tests is that in order to fix some other arg issues, these tests will likely start failing because they never should have been working in the first place.

I believe unknown flag: --version is the correct output when invoking the sub command with the --version flag. So an alternative to deleting the tests, we could explicitly test that it fails (although this doesn't seem like too important of a test to me).

Any thoughts on this? Can I open a PR to delete these two unit tests? Are there other unit tests I could create that would be more helpful?

[marckhouzam]

Thanks for the clear explanation @brianpursley . I think we should first decide what the --version flag should do and see if we have a bug here.

For a good user experience, how should the --version flag behave?

[brianpursley]

My first reaction is that I would not expect --version to work on anything other than the root command.

But I guess if you make it a persistent flag, it can always be shadowed by a sub command if the sub command needs to have its own implementation of --version.

I don't feel too strongly either way on this. Do you have a preference @marckhouzam?

[umarcor]

IIRC, cobra (or pflag) don't support passing args before a subcommand, or mixing args subcommand args. Particularly, I don't think a convention exists to interpret the args before the subcommand as belonging to either the parent or the subcommand itself. Such decision ought to be documented if cobra supported such use case.

Hence, those tests should be changed to expect an error and be renamed so that "version" is not a relevant part of the test (it's just one arg, any). However, if this is due to cobra/pflag not accounting for the non-supported use cases, having it fixed might be a rabbit hole and we'd better just remove them and stick to the user guide.

[github-actions[bot]]

The Cobra project currently lacks enough contributors to adequately respond to all issues. This bot triages issues and PRs according to the following rules:

• After 60d of inactivity, lifecycle/stale is applied. - After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied and the issue is closed. You can:
• Make a comment to remove the stale label and show your support. The 60 days reset. - If an issue has lifecycle/rotten and is closed, comment and ask maintainers if they'd be interested in reopening