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

When persistent flag is shadowed, help text does not show shadowing flag #1651

Closed
marckhouzam opened this issue Mar 31, 2022 · 6 comments · Fixed by #1776
Closed

When persistent flag is shadowed, help text does not show shadowing flag #1651

marckhouzam opened this issue Mar 31, 2022 · 6 comments · Fixed by #1776
Assignees
Labels
area/flags-args Changes to functionality around command line flags and args kind/bug A bug in cobra; unintended behavior

Comments

@marckhouzam
Copy link
Collaborator

If I declare a flag --flag on the root command then declare the same local flag on a sub command, the local flag shadows the persistent one. This seems reasonable. However the help command does not respect this and gives priority to persistent flags.

Here is a program to illustrate:

package main

import (
	"fmt"

	"github.com/spf13/cobra"
)

type testOpts struct {
	root string
	sub   string
}

var opt testOpts

var root = &cobra.Command{
	Use: "prog",
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Printf("root called with root flag %s\n", opt.root)
		fmt.Printf("root called with sub flag %s\n", opt.sub)
	},
}

var sub = &cobra.Command{
	Use: "sub",
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Printf("sub called with root flag %s\n", opt.root)
		fmt.Printf("sub called with sub flag %s\n", opt.sub)
	},
}

func main() {
	root.AddCommand(sub)
        // Persistent flag named --flag
	root.PersistentFlags().StringVar(&opt.root, "flag", "defaultroot", "usage for root")
        // Local flag also named --flag: it will shadow the above flag
	sub.Flags().StringVar(&opt.sub, "flag", "defaultsub", "usage for sub")

	root.Execute()
}

First we check what happens to the flag behaviour:

$ go build -o prog overrideArg.go
$ ./prog --flag MARC
root cmd with root flag MARC and sub flag DEFAULTSUB
# here it is the persistent flag that gets set, as expected

$ ./prog sub --flag MARC
sub cmd with root flag DEFAULTROOT and sub flag MARC
# here it is the local shadowing flag that gets set

$ ./prog --flag MARC sub
sub cmd with root flag DEFAULTROOT and sub flag MARC
# here too it is the local shadowing flag that gets set

This seems right: the more specialized flag takes precedence.

I don't know if this is just by chance in how pflag behaves. I wasn't aware this was possible.

The problem now, is that the help does not respect this behaviour:

./prog help
Usage:
  prog [flags]
  prog [command]

Available Commands:
  completion  Generate the autocompletion script for the specified shell
  help        Help about any command
  sub

Flags:
      --flag string   usage for root (default "DEFAULTROOT")
  -h, --help          help for prog

Use "prog [command] --help" for more information about a command.

# As expected, the persistent flag is documented above but...

$ ./prog help sub
Usage:
  prog sub [flags]

Flags:
  -h, --help   help for sub

Global Flags:
      --flag string   usage for root (default "DEFAULTROOT")

# Here the shadowing flag is not shown.  The user therefore does not realize the behaviour may be different than for the global flag
@marckhouzam marckhouzam added kind/bug A bug in cobra; unintended behavior area/flags-args Changes to functionality around command line flags and args labels Mar 31, 2022
@marckhouzam
Copy link
Collaborator Author

This problem was reported in kubectl where they shadow persistent flags (probably by mistake):
kubernetes/kubernetes#103769

I've traced the problem to

https://github.com/spf13/cobra/blob/master/command.go#L1502-L1506

where we determine the list of local flags for the help by checking if a flag is not part of the parent. That approach gives priority to persistent flag instead of to local flags.

@github-actions
Copy link

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 interseted in reopening

@marckhouzam
Copy link
Collaborator Author

Still good

@github-actions
Copy link

github-actions bot commented Aug 1, 2022

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 interseted in reopening

@eddiezane
Copy link

Still an issue.

@brianpursley
Copy link
Contributor

/assign

hoshsadiq pushed a commit to zulucmd/zulu that referenced this issue Dec 31, 2022
This fixes a bug where a child flag that shadows (has the same
name as) a parent persistent flag would not be shown in the
child command's help output and the parent flag would be shown
instead under the global flags section.

This change makes the help output consistent with the
observed behavior during execution, where the child flag is
the one that is actually used.

Fixes spf13/cobra#1651

Merge spf13/cobra#1776
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/flags-args Changes to functionality around command line flags and args kind/bug A bug in cobra; unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants