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

Added support for option value from environment variables #1266

Closed

Conversation

flitbit
Copy link
Contributor

@flitbit flitbit commented May 25, 2020

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 use commander 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:

#!/usr/bin/env node

// This is used as an example in the README for:
//    Environment variables and options
//
// Example output pretending command called pizza-options (or try directly with `node options-from-env.js`)
//
// $ DEBUG_LEVEL=verbose pizza-options
// { DEBUG_LEVEL: 'verbose', small: undefined, pizzaType: undefined }
// pizza details:
// $ pizza-options -p
// error: option '-p, --pizza-type <type>, env:FAVOURITE_PIZZA' argument missing
// $ DEBUG_LEVEL=info pizza-options -s -p vegetarian
// { DEBUG_LEVEL=info, small: true, pizzaType: 'vegetarian' }
// pizza details:
// - small pizza size
// - vegetarian
// $ FAVOURITE_PIZZA=pepperoni pizza-options --small
// pizza details:
// - small pizza size
// - pepperoni
// $ FAVOURITE_PIZZA=pepperoni pizza-options -s -p cheese
// pizza details:
// - small pizza size
// - cheese

// const commander = require('commander'); // (normal include)
const commander = require('../'); // include commander in git clone of commander repo
const program = new commander.Command();

program
  // environment variables can be declared alone
  .option('env:DEBUG_LEVEL', 'debug level from environment')
  .option('-s, --small', 'small pizza size')
  // environment variables are more interesting when they relate to a command line option
  .option('-p, --pizza-type <type>, env:FAVOURITE_PIZZA', 'flavour of pizza');

program.parse(process.argv);

if (program.DEBUG_LEVEL) console.log(program.opts());
console.log('pizza details:');
if (program.small) console.log('- small pizza size');
if (program.pizzaType) console.log(`- ${program.pizzaType}`);

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.

  • All tests pass
  • 3 additional tests
  • Two more examples
  • Updated English readme
  • Updated comments in the typings

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 example examples/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:

#!/usr/bin/env node

// Commander toggles flags when they are specified multiple times, this
// includes when the value is specified via environment variable.
//
// When specifying a boolean flag from an environment variable, it is necessary
// to coerce the value since environment variables are strings.
//
// Example output pretending command called toggle (or try directly with `node options-flag-from-env.js`)
//
// $ toggle
// disabled
// $ toggle -e
// enabled
// $ toggle -ee
// disabled
// $ ENABLE=t toggle
// enabled
// $ ENABLE=t toggle -e
// disabled
// $ ENABLE=f toggle --enable
// enabled

// const commander = require('commander'); // (normal include)
const commander = require('../'); // include commander in git clone of commander repo
const program = new commander.Command();

function envBoolCoercion(val, prior) {
  if (val !== undefined) {
    return [true, 1, 'true', 't', 'yes', 'y', 'on'].indexOf(
      typeof val === 'string' ? val.toLowerCase() : val
    ) >= 0;
  }
  return !prior;
}

program
  .option('-e, --enable, env:ENABLE', 'enables the feature', envBoolCoercion);

program.parse(process.argv);

console.log(program.enable ? 'enabled' : 'disabled');

ChangeLog

Added support for option value from environment variables

@shadowspawn
Copy link
Collaborator

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);
% node pizza.js 
undefined
% node pizza.js -p cheese
cheese
% FAVOURITE_PIZZA=meat node pizza.js 
meat
1266 % FAVOURITE_PIZZA=meat node pizza.js -p mexican
mexican

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

@shadowspawn
Copy link
Collaborator

(On a side note, I was unable to reproduce the "toggle" behaviour you described when specify a flag multiple times.)

@shadowspawn
Copy link
Collaborator

(I was amused and slightly dismayed at the number of spelling mistakes you also fixed, as most of them were by me! Thanks.)

@flitbit
Copy link
Contributor Author

flitbit commented May 26, 2020

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.

@flitbit
Copy link
Contributor Author

flitbit commented May 26, 2020

(On the spelling... twas the vscode spell-check module, not my attentiveness :o). BTW, is it intentionally British English?

@shadowspawn
Copy link
Collaborator

And thanks for the contribution! I'll hopefully go deeper this weekend.

@shadowspawn
Copy link
Collaborator

I did some research on what other packages do.

Python argparse reportedly does not support environment variables, and a suggested approach is to pass them in as the default value yourself:

oclif has an option flag env for specifying the environment variable to read default from:

curl lists some environment variables in the description of the option (e.g. CURL_CA_BUNDLE) and some in a separate ENVIRONMENT section in the man page (e.g. http_proxy):

yargs has a routine to enable environment variables for all options, with an optional prefix:

@shadowspawn
Copy link
Collaborator

BTW, is it intentionally British English?

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!

@shadowspawn
Copy link
Collaborator

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.

@shadowspawn
Copy link
Collaborator

(Fixing the typos separately and listing @flitbit as co-author.)

@shadowspawn shadowspawn mentioned this pull request May 30, 2020
@shadowspawn
Copy link
Collaborator

Related to documenting env: #682

@lbontecou
Copy link

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.

@backbone87
Copy link

backbone87 commented May 4, 2021

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"

@shadowspawn
Copy link
Collaborator

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 .option(). At the time it was difficult to add anything to options because the API was not extensible. Since then, I have enhanced the Option class to allow additions without having to jam it into the .option() call, so that offers another approach. See #1331

@backbone87
Copy link

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.

@shadowspawn
Copy link
Collaborator

shadowspawn commented May 5, 2021

so you are open to integrating the env vars more tightly with the current codebase?

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.

do you have a preference for a specific API model?

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 optionDescription().

i would like to help create an MR for the current codebase, based on this MR.

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:

  • is the custom processing function called if the environment variable is used? (Probably yes, since it is a text value the same as the value from the command-line.)
  • is the env separate from the default in the help? (Probably yes, and you mentioned "hiding" the default as an issue so I expect this is something you want!)
  • when in the runtime is the env var applied? (Probably in addOption like the default value.)
  • how are boolean values set from the env var? What effect does VALUE=hello have, true, or false, or ignored?
  • subtle: does _concatValue change? (Maybe, but it is already a bit magic and might be hard to achieve. Used for variadic options.)

@backbone87
Copy link

do you have an idea how "env only" options fit with your proposal? new Option(undefined, 'flavour of pizza').env('FAVOURITE_PIZZA') looks weird

@shadowspawn
Copy link
Collaborator

No, I was not considering "env only" options, and they don't fit in as well. Are they something you want?

@backbone87
Copy link

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.

@shadowspawn
Copy link
Collaborator

Hmm, that might take a whole rethink...

@jreusch
Copy link

jreusch commented Aug 10, 2021

tbh having the environment variable support would be a great thing.
i've missed this for a long time and in every place where i use commander it is somehow frankensteined in it.
i'd not consider having the "env only" option a blocking issue on this, rather let's have it in at some point sooner than later...
if it is running inside a container where only env-vars are used, there's no harm in having also the CLI flags (which still i'd think is the primary usecase for commander).
but having this finally in would greatly enhance the usefulness for container usecases (which also is the area i'm missing it).
also the Option class .env()way you proposed looks great to me.

@shadowspawn
Copy link
Collaborator

Having a go at .env() for adding to an option (not for env-only).
#1587

@shadowspawn
Copy link
Collaborator

shadowspawn commented Aug 22, 2021

@jreusch and any other people following this issue:
Feedback welcome on #1587, see if it would help with your use cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants