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

fix: ignore unset environment variables #536

Merged
merged 1 commit into from Oct 31, 2022

Conversation

amcaplan
Copy link
Contributor

@amcaplan amcaplan commented Oct 23, 2022

When an env is set for a boolean flag, if the user doesn't actually specify the value in their ENV, it ends up being treated as though they'd specified false. I don't think this is the correct behavior; instead, it should be considered unspecified.

This is especially relevant in 2 cases:

  1. Boolean flags with default: true. As soon as you add an env config, they now effectively default to false.
  2. Boolean flags with mutual exclusivity. If you specify one, oclif thinks you've specified both, and the command fails. You can reproduce easily:
import {Command, Flags} from '@oclif/core'

export default class Repro extends Command {
  static description = 'Reproduce exclusive flags bug'

  static flags = {
    exclusive1: Flags.string({
      required: false,
      env: 'SOME_ENV_VAR_1',
      exclusive: ['exclusive2'],
    }),
    exclusive2: Flags.boolean({
      required: false,
      env: 'SOME_ENV_VAR_2',
      exclusive: ['exclusive1'],
    }),
  }

  static args = []

  async run(): Promise<void> {
    const {flags} = await this.parse(Repro)
  }
}
$ bin/dev repro
# everything works fine

$ bin/dev repro --exclusive1
Error: The following error occurred:
  --exclusive1=true cannot also be provided when using --exclusive2
# ... stack trace ...

$ bin/dev repro --exclusive2
Error: The following error occurred:
  --exclusive2=true cannot also be provided when using --exclusive1
# ... stack trace ...

@amcaplan amcaplan changed the title Ignore unset environment variables fix: ignore unset environment variables Oct 23, 2022
},
})
expect(out.flags.foo).to.be.true
})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be 100% clear, both of these tests fail when run against main, because in both cases the absent env var is interpreted as falsey:

  2 failing

  1) parse
       env
         boolean
           ignores unset environment variables:
     AssertionError: expected false to be undefined
      at Context.<anonymous> (test/parser/parse.test.ts:921:36)

  2) parse
       env
         boolean
           uses default when environment variable is unset:

      AssertionError: expected false to be true
      + expected - actual

      -false
      +true
      
      at Context.<anonymous> (test/parser/parse.test.ts:931:36)

amcaplan added a commit to Shopify/cli that referenced this pull request Oct 24, 2022
…ive flags

Apparently these never worked anyway for boolean flags before oclif 1.16.1.
Then they started working, but only to specify true; otherwise, they
essentially force a false as though it were specified.

More details: oclif/core#536
amcaplan added a commit to Shopify/cli that referenced this pull request Oct 24, 2022
…ive flags

Apparently these never worked anyway for boolean flags before oclif 1.16.1.
Then they started working, but only to specify true; otherwise, they
essentially force a false as though it were specified.

More details: oclif/core#536
amcaplan added a commit to Shopify/cli that referenced this pull request Oct 24, 2022
Upgrading oclif introduced bugs with boolean flags. Need to downgrade
again until oclif/core#536 is merged to oclif.
amcaplan added a commit to Shopify/cli that referenced this pull request Oct 24, 2022
Upgrading oclif introduced bugs with boolean flags. Need to downgrade
again until oclif/core#536 is merged to oclif.
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

2 participants