-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Added support for option value from environment variables #1266
Conversation
Commander can currently be used with environment variables by specifying them as the default value. For example: const { program } = require('commander');
program
.option('-p, --pizza-type <type>', 'flavour of pizza', process.env.FAVOURITE_PIZZA);
program.parse();
console.log(program.opts().pizzaType);
Does this do enough of what you want? (We could improve the documentation coverage, as only hinted at in the README and does not appear in any examples!) I worked on improving boolean support for this pattern in Command v3: #928 #987 |
(On a side note, I was unable to reproduce the "toggle" behaviour you described when specify a flag multiple times.) |
(I was amused and slightly dismayed at the number of spelling mistakes you also fixed, as most of them were by me! Thanks.) |
I appreciate your suggestion of placing environment variables in place of the default value. I'll add that this is one of many solutions I've employed over the past decade working with Commander. It works as you say, however, when I'm writing a program to be used by others, which is nearly always the case, I have to hobble together help, descriptions, etc. to communicate how my program is intended to be used from the environment. What I wanted to achieve, similar to Commander's whole purpose, was least-effort for program authors and automatic feature discoverability (docs) for program operators. My simple solution is to elevate environment variable handling to a first-class concern in the least disruptive way. My implementation makes environment variable handling an intentional feature of the library, without redefining or obstructing default values, and in a simplistic manner, a regular part of the program's documentation. All that said, I probably should have opened an issue before a PR so we could have the discussion... but it is my way to offer solutions, not problems. |
(On the spelling... twas the vscode spell-check module, not my attentiveness :o). BTW, is it intentionally British English? |
And thanks for the contribution! I'll hopefully go deeper this weekend. |
I did some research on what other packages do. Python
|
Not intentional, just what contributors have used. I have done a lot of updates and type British English spelling by habit so likely a source of some! |
I appreciate the detail in the Pull Request, with background and discussion on the problem. Adding the env to the flags is an interesting approach. It does achieve your goal of making it easy to visibly add support for env. I think it is noisy though, not adding enough value, and not a commonly used pattern. If you want to see if there is community interest for improved support for env, please feel free to open an issue. Thank you for your contributions. |
(Fixing the typos separately and listing @flitbit as co-author.) |
Related to documenting env: #682 |
As a devops engineer trying to create a CI workflow for a developers tool, I was really disappointed to learn this feature didn't exist. |
while using env vars as default values is an easy start to get going, it still lacks documentation (like already mentioned) and also hides away "real" default values in the help section, when you look at the help with some env vars set edit: I also don't really understand the reasoning behind the rejection, since 3 of the 4 packages you looked at for reference, actually do have a tighter integrated support for env vars. also the MR itself is like you said very much "up to standard" |
If anyone would like more visibility on this, feel free to open an issue with a description of the problem. I check the open issues to see if they are accumulating positive feedback (👍 ❤️) but don't regularly check closed issues or Pull Requests. There have been three additional people liked and/or commented on this PR in the last year. I do get notified about comments so this response prompted by @backbone87 One reason I closed this PR was that I did not like adding the environment variable as text into the "flags" parameter to |
thank you for the quick response. so you are open to integrating the env vars more tightly with the current codebase? do you have a preference for a specific API model? i would like to help create an MR for the current codebase, based on this MR. |
I do not wish to promote env var support as a major feature, in case that is what you are asking. I am open to adding support for environment variables as an available feature.
I am thinking it could be added to the Option class, like: program
.addOption(new Option('-p, --pizza-type <type>', 'flavour of pizza').env('FAVOURITE_PIZZA')); For the help, in the first instance I would try adding the environment variable to the option description, like the default value, in
I think there will be almost no common code between this MR and what I have in mind. It will be a new implementation and not a rework of this MR. Things which will come up as implementation decisions:
|
do you have an idea how "env only" options fit with your proposal? |
No, I was not considering "env only" options, and they don't fit in as well. Are they something you want? |
I think the "env only" feature could be considered more important (at least for me) in today's environment where a lot of CLI tools are also executed in containers which are commonly configured via env vars. |
Hmm, that might take a whole rethink... |
tbh having the environment variable support would be a great thing. |
Having a go at |
Pull Request
Problem
Commander does not have direct support for specifying options coming from environment variables, nor does it initialize options from environment variables.
Specifying program options via the environment is commonplace for twelve factor apps, particularly those that run in containers. It would be convenient if when specifying program options, we could also specify that those options may be present in the environment.
Since
commander
does not include this ability, there are two types of options available. One is to usecommander
as designed and move the responsibility to the operator (interpolate environment variables onto the command line). Another option is to introduce another pre or post-processing step that handles environment variables in a special way. I have implemented this second option several different ways over the past decade and they all feel hacky.Solution
My approach was to extend the definition of option flags to include an additional flag indicating that an option's value may come via the environment.
.option('-c, --context [context], env:CLOUD_CONTEXT', 'specifies an execution context', 'google')
.option('-s, --size <size>, env:SHIRT_SIZE', 'shirt size (XS, S, M, L, XL)', 'L')
If you don't mind reading code, I think the
example/options-from-env.js
file expresses the change well:Even though my solution allows environment variables to be declared alone, it becomes much more useful when combined with short and long flags, custom coercion, and default values. All of these continue to work the way they always have, but now, if there is a value present in the environment, it will be (optionally) coerced and will replace the default value, if one is present, as the option's initial value. Subsequently, if the option is also specified by the operator at the the command line, the option specified on the command line takes precedence over the value from the environment.
This modification does not change the interface/contracts.
The pre-existing feature that toggles bald options, such as
-e, --enable
when they appear multiple times is non-intuitive when used with environment variables. I wrote a special exampleexamples/options-flag-from-env.js
and had to debug for a while to figure out why this special case is so mind-bending. The example bash output clarifies the oddity:ChangeLog
Added support for option value from environment variables