BindPFlags functionality does not seem to match documentation

[therealfakemoot]

I've followed the README's example of how to bind flags into viper, but when I try to use the BindFlags() method versus explicitly binding flags by name, there's some sort of disconnect, the values never make it into viper. Minimal test case below.

package main

import (
	"fmt"

	"github.com/spf13/cobra"
	"github.com/spf13/viper"
)

// RootCmd represents the base command when called without any subcommands
var RootCmd = &cobra.Command{
	Use:   "demo",
	Short: "short description",
	Long: "long description",
	// Uncomment the following line if your bare application
	// has an action associated with it:
	Run: func(cmd *cobra.Command, args []string) {
		//genesis.DumpSettings(l.Term)
	},
	PostRun: func(cmd *cobra.Command, args []string) {
	},
}

// Execute adds all child commands to the root command sets flags appropriately.
// This is called by main.main(). It only needs to happen once to the rootCmd.
func Execute() {
	if err := RootCmd.Execute(); err != nil {
		//l.Term.WithError(err).Error("Root command failed unexpectedly.")
	}
}

func init() {
	cobra.OnInitialize(initConfig)

	// Here you will define your flags and configuration settings.
	// Cobra supports persistent flags, which, if defined here,
	// will be global for your application.
	RootCmd.PersistentFlags().String("cfgFile", ".default", "config file (default is $HOME/.conf.yaml)")
	RootCmd.PersistentFlags().BoolP("debug", "D", false, "Enable debug mode. [POSSIBLE PERFORMANCE IMPLICATIONS]")

	// Cobra also supports local flags, which will only run
	// when this action is called directly.
	//RootCmd.Flags().BoolP("toggle", "t", false, "Help message for toggle")

}

// initConfig reads in config file and ENV variables if set.
func initConfig() {}

func init() {

	// These two invocations DO correctly bind the flags into the config layer.
	//viper.BindPFlag("debug", RootCmd.PersistentFlags().Lookup("debug"))
	//viper.BindPFlag("cfgFile", RootCmd.PersistentFlags().Lookup("cfgFile"))

	// This is the invocation that fails to correctly bind the CLI flags into the config layer.
	viper.BindPFlags(RootCmd.Flags())
}

func main() {
	RootCmd.Execute()

	fmt.Println(viper.GetBool("debug"))
	fmt.Println(viper.GetString("cfgFile"))
	
	// Expected output:
	// true
	// .default
}

The documentation ( https://godoc.org/github.com/spf13/viper#BindPFlags ) seems to indicate that this should be all that's necessary to allow config value lookups to also reference set flags. I do not discount the possibility that I'm misunderstanding but because the README only covers explicitly binding individual flag values I can't be sure.

The confusion here is that the code compiles and runs without complaint using BindPFlags() but values passed via command-line don't seem to be available via viper's Get* methods.

My application will eventually have dozens upon dozens of commands, subcommands, and flag sets with common and uncommon flags between each. I'd like to be able to bind-once and not have to update my bindings any time I move, rename, or add flags.

[juztin]

I too just ran across something similar:

This seem to have been working at some point for me
(did some refactoring and now I can't figure out why it's no longer working, if it ever was):

    cmd.Flags().String("address", "", "some address")
    viper.BindPFlag("address", cmd.Flags().Lookup("address"))

When looking up:

    fmt.Println("flag:", viper.GetString("address"))  // Returns empty
    fmt.Println(c.Flags().GetString("address"))        // Returns value set in 'address'

Update

I discovered this quit working when I used the same flag name in a different command.
Is this expected, can you not reuse flag names across commands

Not sure if this is in the docs somewhere, but changing out cmd.Flags().String(...) with cmd.Flags().StringVar(...) works when using the same name, I just need to have the pointer declared for the var.

[gadelkareem]

Any updates?

[filinvadim]

Any updates?

[pojntfx]

Thanks @juztin for your solution. Just to clarify; if one wants to use the BindPFlags functionality for multiple flags, they have to use one pointer for all flags to bind.

i.e.:

// constants.go (used by both commands)
var (
	serverHostPortFlag string
	configFileFlag     string
)

// cmd1.go
	applyCmd.PersistentFlags().StringVarP(&serverHostPortFlag, serverHostPortKey, "s", constants.DHCPDDHostPortDefault, constants.HostPortDocs)

// cmd2.go
	deleteCmd.PersistentFlags().StringVarP(&serverHostPortFlag, serverHostPortKey, "s", constants.DHCPDDHostPortDefault, constants.HostPortDocs)

[kalzoo]

Ran into this as well. This did not work as I had expected:

err := viper.BindPFlags(cmd.Flags())
fmt.Printf("bind error: %v\n", err)
cobraValue, err := cmd.Flags().GetString("server_socket")

fmt.Printf("Cobra value: %s, err: %v\n", cobraValue, err)
fmt.Printf("Viper value: %s\n", v.GetString("server_socket"))

Result:

$ go run main.go serve --server_socket=127.0.0.1:2222
bind error: <nil>
Cobra value: 127.0.0.1:2222, err: <nil>
Viper value: 127.0.0.1:1111

However, iterating over those flags and binding them individually does work:

cmd.Flags().VisitAll(func (flag *pflag.Flag) {
	v.BindPFlag(flag.Name, flag)
})
fmt.Printf("Viper: %s\n", v.GetString("server_socket"))

$ go run main.go serve --server_socket=127.0.0.1:2222           
Viper: 127.0.0.1:2222

No changes to the flags themselves in between "not working" and "working". These are persistent flags, but not of the Var family.

[GwynethLlewelyn]

Piping in late, but I just wanted to add my .02 here...

I've stumbled upon the same issue as all of you. In my case, I'm not using Cobra or any kind of CLI properly speaking, but rather just launching a Web app service that I intend to configure from a .ini file and, optionally, from command-line flags. So, for instance, I can have a 'default' .ini with the usual configuration, but, if something is broken on my app, I wish to restart the app in debug mode by just using a flag. Conversely, when the .ini changes on disk, I'd love to get on-demand, real-time changing of the parameters, without the need to send a SIGHUP or something like that.

And obviously, I'm assuming a certain (fixed) priority: compile-time values first, then environment, then flags, and finally.ini settings. BindPFlags() is supposed to do all that.

In other words, I'd like to have the exact same functionality as iniflags but with the extra goodies that @spf13's packages provide (iniflags works quite well, BTW, no surprises there, but it seems to be little maintained these days, while @spf13 is keeping all his code up to date).

To add to the confusion, I'm an amateur programmer, who spent way too long tinkering with PHP for her own good :) One of the 'bad habits' I picked up was to get all flags inside some sort of meta-structure — just to be aware that some variables are 'special' in some way and not 'regular' variables. Sometimes I use string maps — when I know in advance that all flags will be strings — but it's more often to have just a Config struct, where each configuration parameter is addressed separately, e.g. *Config.HostName and *Config.Port — that way, for me, it's visually easier to know which are configuration options and which are not.

Storing configuration items in string maps has the advantage that tricks such as the one provided by @kalzoo would work nicely — at the cost of forcing every configuration item to be a string. Granted, I could have a map of interfaces... but one ought to wonder why I'm duplicating the very same internal configuration that Viper is using...? Wouldn't it make sense to simply use the map that Viper already uses? (sure, it's just pointers, so no memory is wasted except a few bytes of overhead, but still...)

Here is what ultimately broke everything for me:

To keep command-line flags with .ini configurations in sync, iniflags does not have a very explicit way of creating structured configuration variables. Viper, by contrast, is all about structure; and when using external files saved to disk, be they JSON, YAML, TOML, or even the humble .ini, Viper assumes that there is a structure; if not, there is (in the code, but not really well documented...) a 'parent' or 'root' node, called DEFAULT. This allows, in some cases, to work 'as if' all configuration flags are on a flat organisation, and not in a structured tree.

Well, here's the catch... flags (or pflags) do not really work with 'structured' command-line flags; while there is some magic that can be done to address that (mostly through Cobra), in most cases, one would assume that all command-line flags are on the same level; therefore, when reading them from disk, they would also have to be on a single level, even if the actual configuration file language allows for complex structures.

However, when using .ini files (and presumably other formats, too), Viper hooks into a standard .ini library, which always assumes that there are sections. You can have the same parameter in different sections, and Viper, accordingly, will distinguish both, usually using dot notation such as Section1.variable and Section2.variable. In other words, when reading from a file, Viper will always retrieve a tree-structured configuration. Even labelling a section as [DEFAULT] inside a .ini file will just create a DEFAULT.variable; and, conversely, if you have no sections on the .ini file, the .ini library used will silently ignore anything that comes from the file, and Viper will assume that the file is 'empty' (and therefore ignore whatever variables are being set in it).

So let's assume the following scenario:

Under (p)flags, I define flags --hostname, --port, --scheme and so forth. They have reasonable defaults (or not) set via the (p)flags interface (assigning them to Go variables, structs, or string/interface maps according to one's preferred choice). In my .ini file, I define something like this:

[configuration]
hostname = "my.host.name"
port = 3133 # integer
scheme = "HTTP://"

... and let's assume I invoke my compiled app with myapp --port=3123

Viper will start by adding hostname, port, scheme to its internal structure with whatever defaults have been set via (p)flags. So far, so good; using viper.AllSettings() — which returns a map[string]interface{} — it's easy to see that these settings are all correctly set at the 'root' level.

But when reading from the .ini file, the above-mentioned flags become configuration.hostname, configuration.port and configuration.scheme instead! These are obviously not the same as hostname, port, scheme, so Viper assumes_ that these are new settings, and, as such, populates its internal structure with a new node ('configuration') and places what it assumes to be new variables inside that node.

From the perspective of the programmer, we now have two branches on the tree, one that is being updated through command-line flags (fixed at run-time) and resides at the tree root; and another that gets updated via configuration files (which can be dynamically changed by writing new values on the file), under a subtree/new node. The names at the leaves are identical, but, of course, Viper has no problem with that. It's just the programmer that needs to handle that.

My admittedly naïve approach was simple: I don't care what Viper thinks that its own internal structure should be. Instead, I just use my own structure instead. When reading from (p)flags, I assume a 'flat' structure; when reading values coming from the .ini parsing, I use viper.GetString() with the structured name (e.g. configuration.hostname) but assign the values returned by Viper to my own existing configuration struct. This should work, since Viper never touches my 'internal' config struct, while, on the other hand, I don't change Viper's internal structures directly (and mostly I'm reading from those anyway).

The theory is that I could manually check if a configuration string is present in the .ini file and initialise the corresponding variable; when binding to (p)flags, whatever was passed on the command line would override what's in the .ini.

Alas, whatever approach I use, 'nothing works' — in other words, because configuration variables are inherently different between .ini and (p)flags, Viper does not understand the correspondence between them, even if I'm actually storing its values in my own variables.

Why?

My guess is that 'my' configuration struct is basically just pointers — pointing to content stored somewhere inside Viper's structures. When new values are loaded by Viper (via .ini), I would expect that my pointers would point to the new values just loaded, but that's not quite what happens. On the backstage, Viper is shuffling pointers around, and WIPAINWIG ('what I point at is not what I get'). Sometimes, the pointers are not changed, thus pointing to the 'old' defaults. Sometimes, they're pointing to the values inside the .ini file even though the command-line flags ought to override those values. Sometimes Viper may not be loading the .ini file at all (especially if it has some kind of error). Sometimes, Viper behaves as if it had just reset all initial values (all pointers point to nil or eventually to an empty string). Also, to make matters even more confusing, sometimes Viper correctly initialises a variable which had a default value defined via (p)flags, but then 'forgets' what that default was supposed to be, and, when loading a .ini file, it does not override the variable's value (which is correct only if that variable has been previously set by (p)flags!), or points it to something unexpected (including an empty string!). And, on top of everything, what to say about unpredictable results? (i.e. sometimes it works as intended; sometimes it doesn't, and no code has been changed and/or recompiled!)

This is just too much for my meagre skills to figure out what exactly is happening backstage. The plain, simple truth is that Viper, working in these mysterious ways, is not usable for my own purposes; I might have to switch back to iniflags again (even taking into account the loss of so many nifty features provided by the Viper package(s)...), which I'm not fond at all to do...

[sagikazarmark]

LOL, that's a giant wall of text.

To answer the original issue: I think RootCmd.Flags() and RootCmd.PersistentFlags() are two entirely separate FlagSets, so binding one while flags are defined in the other will probably not work.

Another important thing (I guess this could be better highlighted in the docs) is that binding a FlagSet will only bind the existing flags in that set. Any flags defined later won't be bound.

To your point @GwynethLlewelyn:

The issue you raise has nothing to do with BindPFlags as far as I can tell.

Yes, Viper is somewhat opinionated, but that's what makes it simple at the same time.

As far as I see your problem is with INI section names being part of the config key, thus not matching with your flag names. This seems to be the logical behavior to me though.

Personally, I'd suggest avoiding INI all together. Use a sane format, like JSON or YAML.

Lastly, I'm not sure I understand your explanation about pointers. Everything you described seems to work like it should be even if it doesn't match your expectations. If you have a reproducible problem, please open a separate issue with sufficient details that allows us to debug it. Thank you!

---

I'm closing this issue because as far as I can tell the problems described here are user errors. If you think you found a bug a though, feel free to open a new issue with more details. Thanks!

[GwynethLlewelyn]

Just for the record, my own issue was solved, thanks to this comment:

[...]
As far as I see your problem is with INI section names being part of the config key, thus not matching with your flag names. This seems to be the logical behavior to me though.

Personally, I'd suggest avoiding INI all together. Use a sane format, like JSON or YAML.

You're absolutely right!

Four years ago or so, when I first used Viper, I was using TOML (not much saner than INI, but slightly better). Everything worked back then, so — I thought — why not go for INI this time? After all, the differences are minimal.

Well, guess what: they are not minimal at all. By switching from viper.SetConfigType("ini") to viper.SetConfigType("toml"), everything started working exactly as I expected it to work.

Thanks so much for your suggestion! 🙏

(Granted, I could drop TOML — which is not perfect, by far — and go with JSON or YAML, but in my particular scenario, the config.ini file is shared by applications, and the 'other' apps cannot be changed to use any other format. Fortunately, TOML is just flexible enough to accommodate the slight quirkinesses between different .ini implementations... and works flawlessly with Viper!)

[zboralski]

When you bind individual flags using viper.BindPFlag, you're telling Viper to associate each flag with a specific key in the configuration file.

However, when you bind all of the flags at once using viper.BindPFlags, Viper is trying to infer the correct keys based on the flag names.

For example:

	viper.BindPFlag("campaign.name", campaignCmd.PersistentFlags().Lookup("name"))
	viper.BindPFlag("campaign.description", campaignCmd.PersistentFlags().Lookup("description"))

will expects the following configuration:

campaign:
  name: test
  description: test

using:

viper.BindPFlags(campaignCmd.PersistentFlags())

would expect the following configuration:

description: test
name: test