fix: load env-file at start of flag parsing #28127
Conversation
devsnek
force-pushed
the
parse-env-earlier
branch
from
0240f04 to
20967ef
Compare
devsnek
force-pushed
the
parse-env-earlier
branch
from
20967ef to
8cd77b2
Compare
| @@ -5727,6 +5750,8 @@ fn env_file_arg_parse(flags: &mut Flags, matches: &mut ArgMatches) { | |||
| flags.env_file = matches | |||
| .get_many::<String>("env-file") | |||
| .map(|values| values.cloned().collect()); | |||
|
|
|||
| load_env_variables_from_env_file(flags.env_file.as_ref()); | |||
There was a problem hiding this comment.
I'm not 100% sure about that - this change means that any time we need to construct flags again (for whatever reason) we are going to implicitly load the .env file again. @dsherret wdyt about that?
There was a problem hiding this comment.
well, we read env vars in flag parsing code. so this is kind of unavoidable.
There was a problem hiding this comment.
We should probably separate env var loading from flag parsing. Probably it should be:
- Flag parsing
- Load env file
- Resolve the value with the env vars in CliOptions
There was a problem hiding this comment.
but flag parsing depends on environment variables. another example is DENO_JOBS.
There was a problem hiding this comment.
Yeah, I think we should separate it so that's not the case.
There was a problem hiding this comment.
I don't really understand what you have in mind.
There was a problem hiding this comment.
I think we should leave clap for parsing cli args (not env vars) Three stages: arg parsing with clap, env file loading based on args, then at this point check the value of the env vars (could be backfilled into the flags struct or resolved in CliOptions). Some properties on the flags structs would need to be None to indicate no argument was provided instead of defaulting to a value. I can show an example tomorrow if that would help.
| @@ -1398,6 +1399,28 @@ pub fn flags_from_vec(args: Vec<OsString>) -> clap::error::Result<Flags> { | |||
| Ok(flags) | |||
| } | |||
|
|
|||
| // use eprintln instead of log::warn because logging hasn't been initialized yet | |||
There was a problem hiding this comment.
This doesn't take into account --log-level=none/error anymore unfortunately (we should fix this before landing).
There was a problem hiding this comment.
i agree it is unfortunate but this isn't the only case of this in this file.
| @@ -5727,6 +5750,8 @@ fn env_file_arg_parse(flags: &mut Flags, matches: &mut ArgMatches) { | |||
| flags.env_file = matches | |||
| .get_many::<String>("env-file") | |||
| .map(|values| values.cloned().collect()); | |||
|
|
|||
| load_env_variables_from_env_file(flags.env_file.as_ref()); | |||
There was a problem hiding this comment.
We should probably separate env var loading from flag parsing. Probably it should be:
- Flag parsing
- Load env file
- Resolve the value with the env vars in CliOptions
env-file values need to be available before flag parsing runs, since flag parsing uses env vars in some places.
Fixes: #27851