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

Proposal: Adding support for environment vars #53

Closed
udondan opened this issue Oct 26, 2019 · 4 comments
Closed

Proposal: Adding support for environment vars #53

udondan opened this issue Oct 26, 2019 · 4 comments

Comments

@udondan
Copy link
Contributor

udondan commented Oct 26, 2019

I'm planning to integrate flaggy into an existing project, replacing the currently used urfave/cli package. This existing program also provides the option to pass in options via environment variables. The env part now could be solved on its own. But I was thinking if it wasn't a cool feature for flaggy itself?

It could look something like this:

flaggy.DefaultParser.EnableEnvFlags()
flaggy.String(&stringFlag, "f", "flag", "A test string flag")

If --flag was not passed to the program, we would use the env var FLAG if it is set.

or

flaggy.DefaultParser.EnableEnvFlagsWithPrefix("MYAPP")
flaggy.String(&stringFlag, "f", "flag", "A test string flag")

With a prefix we would look for MYAPP_FLAG.

Precedence should be clear:

  1. The passed --flag
  2. ENV var FLAG
  3. Default value of the flag

Lowercase flags would always relate to UPPERCASE env vars. Hyphens would be replaced with underscores. CamelCase will be split into separate words. So --someFlag would relate to SOME_FLAG.

By default the feature should be disabled, as I can see it could cause problems in some situations, where flags are named like common bash vars, e.g. HOME, TERM, etc

I'm not sure about subcommand flags. For my use case I would only need to add support for global flags - for functionality that would trigger special functionality on a global level, like MYAPP_DEBUG=1. But IF we want that feature as well for subcommand flags, I guess it will make sense to include the subcommand name into the env var name, to avoid conflicts with flags from other subcommands.

flaggy.DefaultParser.EnableEnvFlagsWithPrefix("MYAPP")
flaggy.String(&stringFlagA, "a", "flagA", "A test string flag (A)")
subcommand := flaggy.NewSubcommand("subcommandExample")
subcommand.String(&stringFlagB, "b", "flagB", "A test string flag (B)")

Corresponding env vars would be:

  • MYAPP_FLAG_A
  • MYAPP_SUBCOMMAND_EXAMPLE_FLAG_B

Happy to work on this. Just wanted to check if you were interested in this feature or would reject it.


I also do like the way urfave/cli has support for custom env vars per flag. But I don't see how we could implement this in a backwards compatible way into flaggy. The function signature of all flag functions would change.

@integrii
Copy link
Owner

I like the way you're thinking of this and definitely agree that it would be a good feature to have. We have had some people ask about the bet way to do this in the past, and I suggested a relatively simple way to solve it by setting a global with the flag's value.:

var myFlag = os.Getenv("MY_FLAG")

If flaggy does not get a flag, it will not change the previously parsed environment variable. However, this still needs added to a help message as a prepend and isn't noted in the help output at all. The only reason this isn't in flaggy is because I wasn't sure of the best clean and intuitive way to solve the issue.

I think it is possible to add an entirely new set of flags that do the flag name to environment variable conversion logic you described. For example, we could create functions named flaggy.StringSliceEnv() instead of just flaggy.StringSlice. This would let people easily indicate which values they wanted to have environment variables. We should just be careful to clearly document the environment variable creation logic so it works as expected and isn't a surprise to the user.

If we get that right, it would be easier to use than specifying flags manually. On the flip side, it does not solve the problem of overriding the environment variable use (as you mentioned with the HOME environment variable.... I am not sure what to do there yet. Maybe that means a new Env function won't be compatible here - or perhaps it means that the Env style functions have an additional parameter to specify the environment variable name.

I actually have two long flights across the USA this week. I will take a stab at this in a branch (tomorrow and Tuesday) and report back. I think I know how to work this into the parsers in a way that makes sense.

@udondan
Copy link
Contributor Author

udondan commented Oct 28, 2019

var myFlag = os.Getenv("MY_FLAG")

Oh dear, I tend to overcomplicate things. Indeed, it is that simple to accomplish. 🤦‍♂

However, this still needs added to a help message as a prepend and isn't noted in the help output at all.

I don't know, not necessarily. I actually am not aware of any program that indicates env var counterparts for flags in the hep message. That's something that can be added to the documentation, or prepend/append help message. Or if the customer wants it in the flag description, it can be simply added to its desc.

I agree with everything you wrote, wrt to flaggy.StringSliceEnv() and an additional parameter for defining the flag name. Makes all sense. But I'm no longer sure it is worth the effort now that you made me realize how simple it is with the default value.

Safe travels! 😸

@udondan
Copy link
Contributor Author

udondan commented Oct 28, 2019

One thing to keep in mind is, that with var myFlag = os.Getenv("MY_FLAG"), the help message would show the value of the env var as default value. This might be confusing, since it is a not a fixed default.

On the other hand this might also be used to the users advantage, e.g.

var flagLang = os.Getenv("LANG")
var flagTime = os.Getenv("LC_TIME")
...

@integrii
Copy link
Owner

I ended up leaving my laptop at the TSA in Seattle, so I didn't get to use my 10+ hours of flight time productively as I had hoped... 😞

Perhaps it should be noted how to deal with environment vars as described above in the documentation. It's not obvious that flags not used by the user don't change from their original values.

I think the Env suffixed functions could be helpful, but maybe it's not worth the extra code complexity... Somehow, I am less motivated to write this feature than I was before my trip. I'll try adding something to the readme.

@integrii integrii closed this as completed Feb 4, 2020
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

2 participants