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

It's not possible to customize environment variables with booleans. #1787

Closed
mshima opened this issue Aug 30, 2022 · 11 comments
Closed

It's not possible to customize environment variables with booleans. #1787

mshima opened this issue Aug 30, 2022 · 11 comments

Comments

@mshima
Copy link
Contributor

mshima commented Aug 30, 2022

It's not possible to customize environment variables parsing like:

FOO=false command

Because the variable value is not forwarded:

commander.js/lib/command.js

Lines 1565 to 1568 in f2aec26

} else { // boolean
// keep very simple, only care that envVar defined and not the value
this.emit(`optionEnv:${option.name()}`);
}

GitHub Actions exposes inputs using the pattern:

INPUT_${option.name().toUpperCase()}=${value}

Where allowed boolean values are ['true', 'True', 'TRUE', 'false', 'False, 'FALSE'];

The proposal is to drop the condition

if (option.required || option.optional) { // option can take a value

And add source argument to argParser() changing:

val = option.parseArg(val, oldValue);

to:

val = option.parseArg(val, oldValue, valueSource);

This would allow something like:

option.argParser((val, oldVal, valueSource) => valueSource === 'env' ? ['false', 'False', 'FALSE'].includes(val) : val);
@shadowspawn
Copy link
Collaborator

I am interested in understanding the use case a bit better. Are you using Commander to generate a metadata file, or to consume environment variables produced from a metadata file?

In the metadata file I think the "booleans" are per YAML which has a wider range: https://yaml.org/type/bool.html

I don't know what environment variable value will be produced from a "boolean" but expect it will be just a pair of possible values (like say TRUE/FALSE).

@shadowspawn
Copy link
Collaborator

You do get custom parsing of the environment variable by declaring the option as taking a value. Say:

program.addOption(new Option('--foo [bool]').env('FOO').argParser(myConvertToBool))

@mshima
Copy link
Contributor Author

mshima commented Aug 30, 2022

I am interested in understanding the use case a bit better. Are you using Commander to generate a metadata file, or to consume environment variables produced from a metadata file?

Both.
Produce metadata:
https://github.com/mshima/commander-github-actions/blob/main/src/dev.ts

Consuming:
https://github.com/mshima/commander-github-actions/blob/main/src/index.ts

In the metadata file I think the "booleans" are per YAML which has a wider range: https://yaml.org/type/bool.html

That's version 1.1.
Version 1.2 is more restrictive https://yaml.org/spec/1.2/spec.html#id2804923

I don't know what environment variable value will be produced from a "boolean" but expect it will be just a pair of possible values (like say TRUE/FALSE).

Yaml boolean are string that needs a parser, they are injected to the environment without been parsed and parsed later at @actions/core:
https://github.com/actions/toolkit/blob/bc4be505973a6a7344bfd71e1b32f77e1755310c/packages/core/src/core.ts#L186-L196

You do get custom parsing of the environment variable by declaring the option as taking a value. Say:

program.addOption(new Option('--foo [bool]').env('FOO').argParser(myConvertToBool))

Actually I am targeting a generic alternative.
https://github.com/mshima/commander-github-actions/blob/main/src/index.ts

program.js:

export function buildCommand(command = new Command()) {
  command.option(...)....
}

cli.js (executable):

import { buildCommand } from './program.js';
const options = buildCommand().parse();

action.js (GitHub action entrypoint):

import { Command } from 'commander-github-actions';
import { buildCommand } from './program.js';

const options = buildCommand(new Command()).parse();

A commander application can become a GitHub Action almost out of the box.

@shadowspawn
Copy link
Collaborator

shadowspawn commented Aug 31, 2022

(Not likely to add a parameter directly to parseArg as it took five years to recover from the last change! See #943. Although it would be a less disruptive change to add another now.)

@shadowspawn
Copy link
Collaborator

(Thanks for the detailed info.)

@shadowspawn
Copy link
Collaborator

I think you can custom process env for a boolean option by adding a listener for the optionEnv:${option.name()} event and overwrite the value written by the built-in processing. The event won't pass the environment value, but you can look that up from process.env yourself.

(Let me know if you would like a proof of concept. I can imagine it working, but have not tried it!)

@shadowspawn
Copy link
Collaborator

shadowspawn commented Sep 2, 2022

There is seldom a need to hook the events these days, but they are available. The order of the events matters. By adding the listener after the option my listener will be called after the default processing, and can overwrite the value with the custom value.

program
  .addOption(new Option('-d, --debug').env('DEBUG'))
  .on('optionEnv:debug', function () {
    // The event does not pass the value for a boolean option, so get it ourselves.
    const rawValue = process.env.DEBUG;
    const value = !['false', 'False', 'FALSE'].includes(rawValue);
    this.setOptionValueWithSource('debug', value, 'env');
  });

program.parse();
console.log(program.opts());
$ DEBUG=FALSE node index.js 
{ debug: false }
$ DEBUG=TRUE node index.js 
{ debug: true }

Note to future readers: if this was used in practice, I expect a subclass of Command would add the listener after adding a boolean option with an envVar.

@mshima
Copy link
Contributor Author

mshima commented Sep 2, 2022

That won’t work for my use case.
GitHub sets an empty value for missing inputs, it shouldn’t change current value
In this case:

$ DEBUG='' node index.js 
{ debug: true }

and breaks default
new Option('-p, --pizza').env('INPUT_PIZZA').default('pereroni')

$ INPUT_PIZZA='' node index.js 
{ pizza: '' }

@shadowspawn
Copy link
Collaborator

shadowspawn commented Sep 2, 2022

GitHub sets an empty value for missing inputs, it shouldn’t change current value

That is an extra problem, separate from your original issue with boolean options, as affects non-boolean options too.

My first thought is to groom process.env before parsing and delete the keys with blank values! Crude but simple and effective.

@mshima
Copy link
Contributor Author

mshima commented Sep 3, 2022

After the discussion, reusing the environment support commander provides is not possible without complicated changes.
I decided to inject a custom config parser, since there is no hook for custom config parser I had to add one.
https://github.com/mshima/commander-github-actions/blob/c278846ae947b3aa97b097ffa411c0d939323cde/src/index.ts#L71
Should I create a new issue to discuss?

@mshima mshima closed this as completed Sep 3, 2022
@shadowspawn
Copy link
Collaborator

The new code in your project treating the GitHub env as a separate parse stage looks nicer than trying to wrangle it into Commander's treatment of environment variables.

Should I create a new issue to discuss?

Rather than discussing here? Yes. I'll make a couple of comments since I looked anyway.

It looks like you are emitting an event from a particular point in the parsing. I don't think this is a general solution for custom config parsing. Different approaches get inserted at different points in the parsing.

There was considerable discussion of config files in a previous issue, which revealed how varied the approaches are:
#1584 (comment)

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