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

feat(cli): support multiple env file argument #26527

Merged
merged 11 commits into from
Nov 17, 2024
48 changes: 37 additions & 11 deletions cli/args/flags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -613,7 +613,7 @@ pub struct Flags {
pub internal: InternalFlags,
pub ignore: Vec<String>,
pub import_map_path: Option<String>,
pub env_file: Option<String>,
pub env_file: Option<Vec<String>>,
pub inspect_brk: Option<SocketAddr>,
pub inspect_wait: Option<SocketAddr>,
pub inspect: Option<SocketAddr>,
Expand Down Expand Up @@ -3775,12 +3775,14 @@ fn env_file_arg() -> Arg {
.help(cstr!(
"Load environment variables from local file
<p(245)>Only the first environment variable with a given key is used.
Existing process environment variables are not overwritten.</>"
Existing process environment variables are not overwritten, so if variables with the same names already exist in the environment, their values will be preserved.
Where multiple declarations for the same environment variable exist in your .env file, the first one encountered is applied. This is determined by the order of the files you pass as arguments.</>"
))
.value_hint(ValueHint::FilePath)
.default_missing_value(".env")
.require_equals(true)
.num_args(0..=1)
Copy link
Member

Choose a reason for hiding this comment

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

should we allow specifying multiple values in a single declaration as well?

Copy link
Contributor Author

@bp7968h bp7968h Nov 15, 2024

Choose a reason for hiding this comment

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

Hi @crowlKats , before this PR the behavior was that if multiple declaration of the same variable exists in the .env file, the first one was applied. For example:

VAR="one"
VAR="two"

For the above single env file, VAR='one' would take effect, and this behavior remains the same while passing multiple env file but according to order of files passed, that is if the above was the last file passed and other preceding file also had VAR in it, then 'VAR='one'' is applied.

Please suggest, if this is something that we are looking or any anything else.

Copy link
Member

Choose a reason for hiding this comment

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

no what i meant multiple files in a single flag call, as in --env-file=.env.one,.env.two in addition to allowing multiple calls to --env-file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, that is achievable using the delimiter flag in clap I guess. Do you want me to update the code @crowlKats ?

.action(ArgAction::Append)
bp7968h marked this conversation as resolved.
Show resolved Hide resolved
}

fn reload_arg() -> Arg {
Expand Down Expand Up @@ -5487,7 +5489,9 @@ fn import_map_arg_parse(flags: &mut Flags, matches: &mut ArgMatches) {
}

fn env_file_arg_parse(flags: &mut Flags, matches: &mut ArgMatches) {
flags.env_file = matches.remove_one::<String>("env-file");
flags.env_file = matches
.get_many::<String>("env-file")
.map(|values| values.cloned().collect());
}

fn reload_arg_parse(
Expand Down Expand Up @@ -7423,7 +7427,7 @@ mod tests {
allow_all: true,
..Default::default()
},
env_file: Some(".example.env".to_owned()),
env_file: Some(vec![".example.env".to_owned()]),
..Flags::default()
}
);
Expand Down Expand Up @@ -7517,7 +7521,7 @@ mod tests {
allow_all: true,
..Default::default()
},
env_file: Some(".example.env".to_owned()),
env_file: Some(vec![".example.env".to_owned()]),
unsafely_ignore_certificate_errors: Some(vec![]),
..Flags::default()
}
Expand Down Expand Up @@ -8165,7 +8169,7 @@ mod tests {
subcommand: DenoSubcommand::Run(RunFlags::new_default(
"script.ts".to_string(),
)),
env_file: Some(".env".to_owned()),
env_file: Some(vec![".env".to_owned()]),
code_cache_enabled: true,
..Flags::default()
}
Expand All @@ -8181,7 +8185,7 @@ mod tests {
subcommand: DenoSubcommand::Run(RunFlags::new_default(
"script.ts".to_string(),
)),
env_file: Some(".env".to_owned()),
env_file: Some(vec![".env".to_owned()]),
code_cache_enabled: true,
..Flags::default()
}
Expand Down Expand Up @@ -8214,7 +8218,7 @@ mod tests {
subcommand: DenoSubcommand::Run(RunFlags::new_default(
"script.ts".to_string(),
)),
env_file: Some(".another_env".to_owned()),
env_file: Some(vec![".another_env".to_owned()]),
code_cache_enabled: true,
..Flags::default()
}
Expand All @@ -8235,7 +8239,29 @@ mod tests {
subcommand: DenoSubcommand::Run(RunFlags::new_default(
"script.ts".to_string(),
)),
env_file: Some(".another_env".to_owned()),
env_file: Some(vec![".another_env".to_owned()]),
code_cache_enabled: true,
..Flags::default()
}
);
}

#[test]
fn run_multiple_env_file_defined() {
let r = flags_from_vec(svec![
"deno",
"run",
"--env-file",
"--env-file=.two_env",
"script.ts"
]);
assert_eq!(
r.unwrap(),
Flags {
subcommand: DenoSubcommand::Run(RunFlags::new_default(
"script.ts".to_string(),
)),
env_file: Some(vec![".env".to_owned(), ".two_env".to_owned()]),
code_cache_enabled: true,
..Flags::default()
}
Expand Down Expand Up @@ -8378,7 +8404,7 @@ mod tests {
allow_read: Some(vec![]),
..Default::default()
},
env_file: Some(".example.env".to_owned()),
env_file: Some(vec![".example.env".to_owned()]),
..Flags::default()
}
);
Expand Down Expand Up @@ -10053,7 +10079,7 @@ mod tests {
unsafely_ignore_certificate_errors: Some(vec![]),
v8_flags: svec!["--help", "--random-seed=1"],
seed: Some(1),
env_file: Some(".example.env".to_owned()),
env_file: Some(vec![".example.env".to_owned()]),
..Flags::default()
}
);
Expand Down
17 changes: 10 additions & 7 deletions cli/args/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1132,7 +1132,7 @@ impl CliOptions {
self.flags.otel_config()
}

pub fn env_file_name(&self) -> Option<&String> {
pub fn env_file_name(&self) -> Option<&Vec<String>> {
self.flags.env_file.as_ref()
}

Expand Down Expand Up @@ -1939,19 +1939,22 @@ pub fn config_to_deno_graph_workspace_member(
})
}

fn load_env_variables_from_env_file(filename: Option<&String>) {
let Some(env_file_name) = filename else {
fn load_env_variables_from_env_file(filename: Option<&Vec<String>>) {
let Some(env_file_names) = filename else {
return;
};
match from_filename(env_file_name) {
Ok(_) => (),
Err(error) => {
match error {

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),
}
}
}
}
}
Expand Down
12 changes: 9 additions & 3 deletions cli/standalone/binary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -659,9 +659,15 @@ impl<'a> DenoCompileBinaryWriter<'a> {
remote_modules_store.add_redirects(&graph.redirects);

let env_vars_from_env_file = match cli_options.env_file_name() {
Some(env_filename) => {
log::info!("{} Environment variables from the file \"{}\" were embedded in the generated executable file", crate::colors::yellow("Warning"), env_filename);
get_file_env_vars(env_filename.to_string())?
Some(env_filenames) => {
let mut aggregated_env_vars = IndexMap::new();
for env_filename in env_filenames.iter().rev() {
log::info!("{} Environment variables from the file \"{}\" were embedded in the generated executable file", crate::colors::yellow("Warning"), env_filename);

let env_vars = get_file_env_vars(env_filename.to_string())?;
aggregated_env_vars.extend(env_vars);
}
aggregated_env_vars
bp7968h marked this conversation as resolved.
Show resolved Hide resolved
}
None => Default::default(),
};
Expand Down
5 changes: 5 additions & 0 deletions tests/integration/run_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,11 @@ itest!(env_file_missing {
output: "run/env_file_missing.out",
});

itest!(env_file_multiple {
Copy link
Member

Choose a reason for hiding this comment

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

Could I actually ask you to rewrite these three env_file_ tests to spec tests? You can see some examples in tests/specs/run/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will do so thank you for pointing it out.

args: "run --env=env --env=env_one --env=env_two --allow-env run/multiple_env_file.ts",
output: "run/multiple_env_file.out",
});

itest!(lock_write_fetch {
args:
"run --quiet --allow-import --allow-read --allow-write --allow-env --allow-run run/lock_write_fetch/main.ts",
Expand Down
2 changes: 2 additions & 0 deletions tests/testdata/env_one
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
FOO=BARBAR
ANOTHER_FOO=OVERRIDEN_BY_ENV_ONE
1 change: 1 addition & 0 deletions tests/testdata/env_two
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
FOO=OVERRIDEN_BY_ENV_TWO
4 changes: 4 additions & 0 deletions tests/testdata/run/multiple_env_file.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
OVERRIDEN_BY_ENV_TWO
OVERRIDEN_BY_ENV_ONE
First Line
Second Line
3 changes: 3 additions & 0 deletions tests/testdata/run/multiple_env_file.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
console.log(Deno.env.get("FOO"));
console.log(Deno.env.get("ANOTHER_FOO"));
console.log(Deno.env.get("MULTILINE"));
Loading