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.

Sign up for GitHub

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: load env-file at start of flag parsing #28127

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: load env-file at start of flag parsing #28127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 27 additions & 2 deletions cli/args/flags.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ use deno_path_util::url_to_file_path;
use deno_runtime::deno_permissions::SysDescriptor;
use deno_telemetry::OtelConfig;
use deno_telemetry::OtelConsoleConfig;
use deno_terminal::colors;
use log::debug;
use log::Level;
use serde::Deserialize;
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't take into account --log-level=none/error anymore unfortunately (we should fix this before landing).

Copy link
Member Author

Choose a reason for hiding this comment

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

i agree it is unfortunate but this isn't the only case of this in this file.

#[allow(clippy::print_stderr)]
fn load_env_variables_from_env_file(filename: Option<&Vec<String>>) {
let Some(env_file_names) = filename else {
return;
};

for env_file_name in env_file_names.iter().rev() {
match dotenvy::from_filename(env_file_name) {
Ok(_) => (),
Err(error) => {
match error {
dotenvy::Error::LineParse(line, index)=> eprintln!("{} Parsing failed within the specified environment file: {} at index: {} of the value: {}", colors::yellow("Warning"), env_file_name, index, line),
dotenvy::Error::Io(_)=> eprintln!("{} The `--env-file` flag was used, but the environment file specified '{}' was not found.", colors::yellow("Warning"), env_file_name),
dotenvy::Error::EnvVar(_)=> eprintln!("{} One or more of the environment variables isn't present or not unicode within the specified environment file: {}", colors::yellow("Warning"), env_file_name),
_ => eprintln!("{} Unknown failure occurred with the specified environment file: {}", colors::yellow("Warning"), env_file_name),
}
}
}
}
}

fn enable_unstable(command: Command) -> Command {
command
.mut_arg("unstable", |arg| {
Expand Down Expand Up @@ -5115,6 +5138,7 @@ fn repl_parse(
flags: &mut Flags,
matches: &mut ArgMatches,
) -> clap::error::Result<()> {
env_file_arg_parse(flags, matches);
unstable_args_parse(flags, matches, UnstableArgsConfig::ResolutionAndRuntime);
compile_args_without_check_parse(flags, matches)?;
cached_only_arg_parse(flags, matches);
Expand All @@ -5125,7 +5149,6 @@ fn repl_parse(
v8_flags_arg_parse(flags, matches);
seed_arg_parse(flags, matches);
enable_testing_features_arg_parse(flags, matches);
env_file_arg_parse(flags, matches);
strace_ops_parse(flags, matches);

let eval_files = matches
Expand Down Expand Up @@ -5691,6 +5714,7 @@ fn runtime_args_parse(
include_inspector: bool,
include_allow_scripts: bool,
) -> clap::error::Result<()> {
env_file_arg_parse(flags, matches);
unstable_args_parse(flags, matches, UnstableArgsConfig::ResolutionAndRuntime);
compile_args_parse(flags, matches)?;
cached_only_arg_parse(flags, matches);
Expand All @@ -5708,7 +5732,6 @@ fn runtime_args_parse(
v8_flags_arg_parse(flags, matches);
seed_arg_parse(flags, matches);
enable_testing_features_arg_parse(flags, matches);
env_file_arg_parse(flags, matches);
strace_ops_parse(flags, matches);
Ok(())
}
Expand All @@ -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());
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

well, we read env vars in flag parsing code. so this is kind of unavoidable.

Copy link
Member

Choose a reason for hiding this comment

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

We should probably separate env var loading from flag parsing. Probably it should be:

  1. Flag parsing
  2. Load env file
  3. Resolve the value with the env vars in CliOptions

Copy link
Member Author

Choose a reason for hiding this comment

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

but flag parsing depends on environment variables. another example is DENO_JOBS.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think we should separate it so that's not the case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really understand what you have in mind.

Copy link
Member

Choose a reason for hiding this comment

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

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.

}

fn reload_arg_parse(
Expand Down
23 changes: 0 additions & 23 deletions cli/args/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ use deno_semver::npm::NpmPackageReqReference;
use deno_semver::StackString;
use deno_telemetry::OtelConfig;
use deno_terminal::colors;
use dotenvy::from_filename;
pub use flags::*;
pub use lockfile::AtomicWriteFileWithRetriesError;
pub use lockfile::CliLockfile;
Expand Down Expand Up @@ -481,8 +480,6 @@ impl CliOptions {
}
}

load_env_variables_from_env_file(flags.env_file.as_ref());

Ok(Self {
flags,
initial_cwd,
Expand Down Expand Up @@ -1321,26 +1318,6 @@ pub fn config_to_deno_graph_workspace_member(
})
}

fn load_env_variables_from_env_file(filename: Option<&Vec<String>>) {
let Some(env_file_names) = filename else {
return;
};

for env_file_name in env_file_names.iter().rev() {
match from_filename(env_file_name) {
Ok(_) => (),
Err(error) => {
match error {
dotenvy::Error::LineParse(line, index)=> log::info!("{} Parsing failed within the specified environment file: {} at index: {} of the value: {}", colors::yellow("Warning"), env_file_name, index, line),
dotenvy::Error::Io(_)=> log::info!("{} The `--env-file` flag was used, but the environment file specified '{}' was not found.", colors::yellow("Warning"), env_file_name),
dotenvy::Error::EnvVar(_)=> log::info!("{} One or more of the environment variables isn't present or not unicode within the specified environment file: {}", colors::yellow("Warning"), env_file_name),
_ => log::info!("{} Unknown failure occurred with the specified environment file: {}", colors::yellow("Warning"), env_file_name),
}
}
}
}
}

/// Gets the --allow-import host from the provided url
fn allow_import_host_from_url(url: &Url) -> Option<String> {
let host = url.host()?;
Expand Down
Loading