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

Same flag name for different cobra commands does not work #233

Open
nii236 opened this issue Aug 30, 2016 · 15 comments
Open

Same flag name for different cobra commands does not work #233

nii236 opened this issue Aug 30, 2016 · 15 comments

Comments

@nii236
Copy link

nii236 commented Aug 30, 2016

I'm not sure if this is expected behaviour or not.

I have a decryptCmd and encryptCmd cobra commands.

They both have flag message.

func init() {
    RootCmd.AddCommand(encryptCmd)
    encryptCmd.Flags().StringP("message", "m", "teststring", "secret unencrypted base64")
    viper.BindPFlags(encryptCmd.Flags())
}
func init() {
    RootCmd.AddCommand(decryptCmd)
    decryptCmd.Flags().StringP("message", "m", "teststring", "base64 encrypted secret")
    viper.BindPFlags(decryptCmd.Flags())
}

Does viper.GetString() work globally across commands or is it command specific? Because when flags have the same name but for different commands then viper can not get the flag value consistently.

@nii236 nii236 changed the title Same flag name for different cobra commands Same flag name for different cobra commands does not work Aug 30, 2016
mook-as added a commit to mook-as/fissile that referenced this issue Apr 4, 2017
This works around issues where viper gets confused when there are mutiple
subcommands with parameters named the same.

See also: spf13/viper#233
@iamsaso
Copy link

iamsaso commented Jul 11, 2017

Any updates on this?

@fermin-silva
Copy link

fermin-silva commented Jul 12, 2017

I think this is the cause:

func (v *Viper) BindFlagValue(key string, flag FlagValue) error {
	if flag == nil {
		return fmt.Errorf("flag for %q is nil", key)
	}
	v.pflags[strings.ToLower(key)] = flag
	return nil
}

If the flag was already bound, it simply overwrites it, and it does it silently.
At the very least it should warn that you are overwriting the flag.

In my case I'm doing

cmd1.PersistentFlags().Int(name, defaultValue, usage)
viper.BindPFlag(name, cmd1.PersistentFlags().Lookup(name))

cmd2.PersistentFlags().Int(name, defaultValue, usage)
viper.BindPFlag(name, cmd2.PersistentFlags().Lookup(name))

so only if I call the second cobra command the flag gets parsed. Otherwise I get the default value

@t00f
Copy link

t00f commented Jul 21, 2017

Having the same issue here. I have a rootCmd that has multiple subcommands. 3 of these subcommands needs the following parameter

subcommand.Flags().StringSliceP("param", "p", nil, "Allow to pass multiple parameters to the command")
viper.BindPFlag("param", subcommand.Flags().Lookup("param"))

As I have 3 commands using the same flag "param", viper.GetStringSlice("param") is empty.

I tried to use Persistant Flags instead - it would work for me as subcommands does not have subcommands - but without success.

Any idea / workaround ?

@gesquive
Copy link

gesquive commented Sep 7, 2017

Wrote a really fast example case for this while debugging it myself.

https://gist.github.com/gesquive/c36501fbd84ecab00d3ff2c9502020f9

Kind of crazy that this bug has been around for a year now.

@himanshugpt
Copy link

Encountered same issue.

@stefansedich
Copy link

stefansedich commented Dec 19, 2017

Just ran into this myself too thought I was going crazy, ended up using the workaround in mook-as/fissile@ebd1ad3 and everything is happy again.

@chris-rock
Copy link

The issue is that init function is executed by go for each command automatically. The following solution allows to us to keep using the global viper, but only register the binds if a command is executed.

var sub1Cmd = &cobra.Command{
   PreRun: func(cmd *cobra.Command, args []string) {
		viper.BindPFlag("message", cmd.Flags().Lookup("message"))
   },
   ...
}

var sub2Cmd = &cobra.Command{
   PreRun: func(cmd *cobra.Command, args []string) {
		viper.BindPFlag("message", cmd.Flags().Lookup("message"))
   },
   ...
}

See https://github.com/spf13/cobra#prerun-and-postrun-hooks for more details.

meyermarcel pushed a commit to meyermarcel/icm that referenced this issue Jun 29, 2018
* Move flag bindings to prerun hooks
  For further information: spf13/viper#233
meyermarcel pushed a commit to meyermarcel/icm that referenced this issue Jul 6, 2018
* Move flag bindings to prerun hooks
  For further information: spf13/viper#233
@prabhatsharma
Copy link

prabhatsharma commented Nov 11, 2018

This workaround is working for me. Bind the flag again in every subCmd

Run: func(cmd *cobra.Command, args []string) {
		
		// workaround for https://github.com/spf13/viper/issues/233
		viper.BindPFlag("user", cmd.Flags().Lookup("user"))

		user := viper.GetString("user")

kostko added a commit to oasisprotocol/oasis-core that referenced this issue Jan 15, 2019
@prologic
Copy link

prologic commented Apr 3, 2019

I got bitten by this too. The documentation/readme should not encourage bad design by abusing init() functions like this. This doesn't work because you are mutating Viper configuration and binding options to keys in viper's state that are dynamically bound to the sub-command being executed. Doing it in init() seems wrong here.

I don't call doing this in PreRiun() a "work-around" at all; but the correct way to bind viper flags (for lack of any other documented accepted way).

Bind your viper flags in PreRun() or Ruin() -- **NOT** init()`.

Blokje5 added a commit to Blokje5/conftest that referenced this issue Oct 9, 2019
… at the same time. See: spf13/viper#233. The solution is to use Prerun in the Cobra command to bind the viper flags at the last possible moment.
Blokje5 added a commit to Blokje5/conftest that referenced this issue Oct 9, 2019
… at the same time. See: spf13/viper#233. The solution is to use Prerun in the Cobra command to bind the viper flags at the last possible moment.
@chen-chao
Copy link

chen-chao commented Nov 13, 2019

I think viper is abused in this case. Local flags are accessible in the local command thus they're not necessarily stored in viper(cobra's readme is a little misleading). viper should be used to store config and global or "trans-command" parameters to avoid name conflicts.

leighmcculloch added a commit to stellar/go that referenced this issue Mar 27, 2020
… across commands (#2424)

### What

Allow multiple flags to be defined with the same name that are attached to different commands.

### Why

Cobra allows the definition of flags on different commands that have the same name. There can be good reasons to do this like two subcommands requiring the same parameter, but other commands not requiring that parameter.

Viper also supports this too, if we bind flags into viper as they're required, but not if we bind them all upfront.

This comment spf13/viper#233 (comment) suggests binding flags to wiper when a command is actually running. This has other advantages, like there are not parameters for other commands bound unnecessarily.

Fix #2421
franciscodiazydiaz added a commit to franciscodiazydiaz/mev-boost-relay that referenced this issue Nov 8, 2022
Consolidate config at /config/variables.go
Fix linting issues
Move bindings to PreRun to workaround spf13/viper#233
Add `Get*` functions to Config
Update all GetEnv calls to use config instead
franciscodiazydiaz added a commit to franciscodiazydiaz/mev-boost-relay that referenced this issue Nov 9, 2022
Consolidate config at /config/variables.go
Fix linting issues
Move bindings to PreRun to workaround spf13/viper#233
Add `Get*` functions to Config
Update all GetEnv calls to use config instead
franciscodiazydiaz added a commit to franciscodiazydiaz/mev-boost-relay that referenced this issue Nov 14, 2022
Consolidate config at /config/variables.go
Fix linting issues
Move bindings to PreRun to workaround spf13/viper#233
Add `Get*` functions to Config
Update all GetEnv calls to use config instead
franciscodiazydiaz added a commit to franciscodiazydiaz/mev-boost-relay that referenced this issue Nov 14, 2022
Consolidate config at /config/variables.go
Fix linting issues
Move bindings to PreRun to workaround spf13/viper#233
Add `Get*` functions to Config
Update all GetEnv calls to use config instead
@figadore
Copy link

Might be misunderstanding how to apply this workaround

var sub1Cmd = &cobra.Command{
   PreRun: func(cmd *cobra.Command, args []string) {
		viper.BindPFlag("message", cmd.Flags().Lookup("message"))
   },
   ...
}

I ran into initialization loop for cmd errors using scaffolded commands and viper.BindPFlags(cmd.Flags()), so I had to define the PreRun function in init

func init() {
	cmd.Flags().BoolP("debug", "d", false, "Whether to enable debugging")
	cmd.PreRun = func(cmd *cobra.Command, args []string) {
		viper.BindPFlags(cmd.Flags())
	}
}

The README says

This means you can bind as early as you want, even in an init() function.

but it doesn't mention this gotcha at all. Does it need an update?

mtardy added a commit to cilium/tetragon that referenced this issue Jun 21, 2023
When using same local flag for different subcommands, binding to the
global configuration will erase previous values with the value of the
last command's flag registered. See spf13/viper#233.

Using a global set of variables to store the flags value of the
command solves the issue. Also add a local variable to store the value
of the output flag of the tracingpolicy list subcommand.

We may want to avoid the usage of the global configuration and using
viper.BindPFlags in the future to avoid this issue to appear again.

Also add checks on the value of flags that are restricted to certain
values.

Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com>
@miparnisari
Copy link

Has someone sent a PR that updates the README? I cannot find any

mtardy added a commit to cilium/tetragon that referenced this issue Jun 23, 2023
When using same local flag for different subcommands, binding to the
global configuration will erase previous values with the value of the
last command's flag registered. See spf13/viper#233.

Using a global set of variables to store the flags value of the
command solves the issue. Also add a local variable to store the value
of the output flag of the tracingpolicy list subcommand.

We may want to avoid the usage of the global configuration and using
viper.BindPFlags in the future to avoid this issue to appear again.

Also add checks on the value of flags that are restricted to certain
values.

Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com>
Jack-R-lantern added a commit to Jack-R-lantern/tetragon that referenced this issue Jul 2, 2023
viper.BindPFlags has a ISSUE spf13/viper#233
replace all viper.BindPFlags

Signed-off-by: Jack-R-lantern <tjdfkr2421@gmail.com>
Jack-R-lantern added a commit to Jack-R-lantern/tetragon that referenced this issue Jul 2, 2023
viper.BindPFlags has a ISSUE spf13/viper#233
replace all viperremove viper.BindPFlags

Signed-off-by: Jack-R-lantern <tjdfkr2421@gmail.com>
Jack-R-lantern added a commit to Jack-R-lantern/tetragon that referenced this issue Jul 2, 2023
viper.BindPFlags has a ISSUE spf13/viper#233
replace all viper.BindPFlags

Signed-off-by: Jack-R-lantern <tjdfkr2421@gmail.com>
Jack-R-lantern added a commit to Jack-R-lantern/tetragon that referenced this issue Jul 2, 2023
viper.BindPFlags has a ISSUE spf13/viper#233
replace all viper.BindPFlags

Signed-off-by: Jack-R-lantern <tjdfkr2421@gmail.com>
Jack-R-lantern added a commit to Jack-R-lantern/tetragon that referenced this issue Jul 2, 2023
viper.BindPFlags has a ISSUE spf13/viper#233
replace all viper.BindPFlags

Signed-off-by: Jack-R-lantern <tjdfkr2421@gmail.com>
Jack-R-lantern added a commit to Jack-R-lantern/tetragon that referenced this issue Jul 11, 2023
viper.BindPFlags has a ISSUE spf13/viper#233
replace all viper.BindPFlags

Signed-off-by: Jack-R-lantern <tjdfkr2421@gmail.com>
Jack-R-lantern added a commit to Jack-R-lantern/tetragon that referenced this issue Jul 11, 2023
viper.BindPFlags has a ISSUE spf13/viper#233
replace all viperremove viper.BindPFlags

Signed-off-by: Jack-R-lantern <tjdfkr2421@gmail.com>
Jack-R-lantern added a commit to Jack-R-lantern/tetragon that referenced this issue Jul 11, 2023
viper.BindPFlags has a ISSUE spf13/viper#233
replace all viper.BindPFlags

Signed-off-by: Jack-R-lantern <tjdfkr2421@gmail.com>
Jack-R-lantern added a commit to Jack-R-lantern/tetragon that referenced this issue Jul 11, 2023
viper.BindPFlags has a ISSUE spf13/viper#233
replace all viper.BindPFlags

Signed-off-by: Jack-R-lantern <tjdfkr2421@gmail.com>
Jack-R-lantern added a commit to Jack-R-lantern/tetragon that referenced this issue Jul 11, 2023
viper.BindPFlags has a ISSUE spf13/viper#233
replace all viper.BindPFlags

Signed-off-by: Jack-R-lantern <tjdfkr2421@gmail.com>
@leakingtapan
Copy link

leakingtapan commented Oct 5, 2023

got bitten by this as well. any plan on address the issue or at least some docs for the workaround?

@ZelvaMan
Copy link

I just spend few hours trying to find why my program did not work.
If this is indeed intended behavior, there should at least be some warning in documentation.
The documentation suggest that you should define flags with cobra and bind them to the command in one place, but this isnt possible with current behavior.

josejulio added a commit to josejulio/inventory-api that referenced this issue Aug 13, 2024
 - Was initially checking spf13/viper#233
   but none of the suggestions there seemed to work.
   In our case it looked like viper or cobra wasn't loading the value at all.
 - Moved some code to the PreRun and that seems to work. Some users suggest
   that the code shouldn't be in the init and use the PreRun instead.
akoserwal pushed a commit to project-kessel/inventory-api that referenced this issue Aug 14, 2024
* RHCLOUD-34576 Moving config reading to PreRun function
 - Was initially checking spf13/viper#233
   but none of the suggestions there seemed to work.
   In our case it looked like viper or cobra wasn't loading the value at all.
 - Moved some code to the PreRun and that seems to work. Some users suggest
   that the code shouldn't be in the init and use the PreRun instead.

* Adding a test to ensure it doesn't happen again
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests