apollographql / rover

The CLI for Apollo GraphOS
https://rover.apollo.dev
Other
409 stars 86 forks source link

Change env variable / cli arg precedence #1047

Open BrynCooke opened 2 years ago

BrynCooke commented 2 years ago

Description

Currently Rover uses the following precedence rules for command line args:

  1. Env variables
  2. Command line args
  3. Defaults

However this is rather unusual. Typically the following is used

  1. Command line args
  2. Env variables
  3. Defaults

This way the user does not get unexpected behaviour if they have an environment variable set and they forget about it. The worst case is that users will try multiple different command line args and conclude they have no effect.

EverlastingBugstopper commented 2 years ago

This would definitely be a breaking change, something we'd want to detect and warn users about ahead of time.

i'm thinking that soon-ish we should start printing warnings if we notice an environment variable exists. there's some maybe funky behavior here around if they don't specify an argument but structopt uses a default. like (--profile default) in most API related commands.

pre-1.0:

$ export APOLLO_KEY="env_var"
$ rover config auth
Enter your API key:
default_profile
$ rover config whoami
WARN: $APOLLO_KEY is overriding `--profile default` in this command.
WARN: In Rover 1.0, the argument will take precedence over the environment variable
Checking identity of your API key against the registry.
Key Type: USER
User ID: gh.EverlastingBugstopper
Origin: $APOLLO_KEY
API Key: env_var
$ rover config auth --profile test
Enter your API key:
test_profile
$ rover config whoami --profile test
WARN: $APOLLO_KEY is overriding `--profile test` in this command.
WARN: In Rover 1.0, the argument will take precedence over the environment variable
Checking identity of your API key against the registry.
Key Type: USER
User ID: gh.EverlastingBugstopper
Origin: $APOLLO_KEY
API Key: env_var

i'm assuming this is how we would want precedence to work, correct me if I'm wrong.

post-1.0:

$ export APOLLO_KEY="env_var"
$ rover config auth
Enter your API key:
default_profile
$ rover config whoami --profile test
Checking identity of your API key against the registry.
Key Type: USER
User ID: gh.EverlastingBugstopper
Origin: $APOLLO_KEY
API Key: env_var
$ rover config auth --profile test
Enter your API key:
test_profile
$ rover config whoami --profile test
Checking identity of your API key against the registry.
Key Type: USER
User ID: gh.EverlastingBugstopper
Origin: --profile test
API Key: test_profile
EverlastingBugstopper commented 2 years ago

This is a quote from clig.dev:

Apply configuration parameters in order of precedence. Here is the precedence for config parameters, from highest to lowest:

1) Flags 1) The running shell’s environment variables 1) Project-level configuration (eg. .env) 1) User-level configuration 1) System wide configuration

ptondereau commented 2 years ago

Definitely agree that flags should have precedence over ENV. From an old PHP dev, some frameworks took the decision to remove some flags and just use an ENV but I'm not a big fan.