Possibility to exclude certain flags from specific "type" (cli, config, env)?

[danvirsen]

I am currently cleaning up an application left by another developer. They merge CLI parameters, ENV variables, and file parameters manually but it had to a lot of inconsistencies where some configuration was only possible via ENV variables, in some cases the ENV was prioritised over the CLI parameter but not always, etc. This package seems to be a great way to keep this symmetrical and protect against issues with inconsistencies in the future. It already saved me a lot of time when I did the initial pass at cleaning up the configuration. I'm using v4.

Unfortunately, I'm having some questions about how the various "types" (for lack of a better word) are being handled. Mainly two things:

1. The short form of a flag can be used as a ENV variable and as a key in the config file
   Having an item named "m" in a config file or as an environment variable isn't very user friendly and I have a feeling that some of our users will use it if they find out and think that it makes the configuration slimmer and neater. I also have a feeling that this will cause some issues when people try to figure out what is actually configured. Right now I'm considering only using long form names, but that does make it fairly tedious to run with CLI parameters.
2. Exclude a flag from a certain type
   It makes no sense to accept flags like "help" or "version" in any other way other than CLI parameters.

I don't think that either of these two issues are unique to our use case.

A bonus issue that could be solved by addressing this is that we currently use the short form -m and -M to set the min and max in a range, and that doesn't work with ff currently.

Do you think it would make sense to:

1. filter out the short form of flags from config and ENV?
2. allow the "type" to be configurable per flag, or as a filter that could be passed into the "With*" functions that are then passed into Parse?

[peterbourgon]

The short form of a flag can be used as a ENV variable and as a key in the config file

I agree that env vars should only be parsed from the "long form" of defined flags. I'll make this change.

Exclude a flag from a certain type It makes no sense to accept flags like "help" or "version" in any other way other than CLI parameters.

I agree in general. But this change would mean adding a new dimension of configuration to every flag, defining which sources of truth the flag may be parsed-from. And that's not really a boolean covering just env vars, it's an enumeration covering every possible source of truth (defaulting to all). It's not obvious to me that the problem solved by this change is significant enough to offset the complexity that would be introduced.

Can you say more about the problem(s) you're currently experiencing, and how this change would fix them?

[danvirsen]

Thanks for the quick reply!

I agree that env vars should only be parsed from the "long form" of defined flags. I'll make this change.

Great, thanks! Will that only be applied for env vars or for config files as well?

Can you say more about the problem(s) you're currently experiencing, and how this change would fix them?

The main problem (if you can call it that) is that we have a "help" and "version" flag. To me it doesn't make much sense to be able to set these in a config file or as an env var since it will print out a value and exit the application. When using a configuration file or env vars we're probably running in a container and this behaviour is bound to cause confusion. It's not really a problem since it's up to the person setting up the application to run it with correct parameters. So it's very much a nice-to-have now that I have slept on it :).

To get around it, we could parse those flags manually. But then we would have to manually add them to the help output. It's nice to have it all in one place.

I created a fork to try out the filter approach and I think it could potentially be a fairly clean feature (if cleaned up). What I did is very much a proof-of-concept that I slapped together in ten minutes so I'm not sure if it makes sense, but it works for my use case.

A filter would then be added like so:

ff.Parse(fs, os.Args[1:],
  ff.WithEnvVars(),
  ff.WithEnvVarFilter(func(f ff.Flag) bool {
    name, _ := f.GetLongName()
    switch name {
    case "help", "version":
      return false
    default:
      return true
    }
  }),
)

[peterbourgon]

To me it doesn't make much sense to be able to set [help or version flags] in a config file or as an env var since it will print out a value and exit the application.

If someone writes version = true in a config file, or runs env YOURCOMMAND_HELP=1 yourcommand ..., then I personally very much prefer that the result would follow the well-defined and consistent set of behavioral rules that already exist. Meaning, the same effect as passing --version=true or --help as explicit flags. That consistency is easier to understand than special-casing specific flags, and doesn't require an additional dimension of complexity in the package API.

So I agree with you that it wouldn't make sense to do these things. But for me that's insufficient justification for the changes you're proposing. Show me how it can happen easily, commonly, and accidentally, and I'm all ears.

[danvirsen]

Yes I agree. Withdrawn 😄 Consistency is after all why I decided to use this package instead of mending our own solution.

[peterbourgon]

🙇

I'll still do the long-form stuff, FWIW