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

Conversation

bp7968h
Copy link
Contributor

@bp7968h bp7968h commented Oct 24, 2024

Related Issue

Fixes #26425

Overview

This PR adds support for specifying multiple environment files as arguments when using the Deno CLI. Subsequent files override pre-existing variables defined in previous files.

If the same variable is defined in the environment and in the file, the value from the environment takes precedence.

Example Usage

deno run --allow-env --env-file --env-file=".env.one" --env-file=".env.two" script.ts

@CLAassistant
Copy link

CLAassistant commented Oct 24, 2024

CLA assistant check
All committers have signed the CLA.

@bp7968h bp7968h force-pushed the support-multiple-env-file-26425 branch from e1ddf39 to 5b01b76 Compare October 24, 2024 21:30
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

@bp7968h looks good, maybe you could add another test in tests/integration/run_tests.rs next to env_file test that uses two --env flags and showcases that the variables are properly overriden.

Note to self: https://docs.deno.com/runtime/reference/env_variables/#.env-file needs to be updated when this PR lands.

cli/standalone/binary.rs Show resolved Hide resolved
cli/args/flags.rs Show resolved Hide resolved
@bartlomieju bartlomieju added this to the 2.1.0 milestone Oct 24, 2024
running 3 tests
test run::env_file ... ok
test run::env_file_missing ... ok
test run::env_file_multiple ... ok

test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 1102 filtered out; finished in 0.78s
…ncreased a number to lint as all env_file test would be in the same place
@bp7968h bp7968h force-pushed the support-multiple-env-file-26425 branch from dc372d5 to a2a512d Compare October 25, 2024 00:56
@crowlKats crowlKats self-requested a review November 12, 2024 15:36
@bartlomieju
Copy link
Member

Hey @bp7968h, we're ready to merge this PR, could you please resolve the conflicts?

@@ -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.

@@ -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 ?

@bartlomieju
Copy link
Member

@bp7968h could you please open a PR to deno-docs repo to update this file: https://github.com/denoland/docs/blob/main/deploy/manual/environment-variables.md#deployment-environment-variables to mention that the flag can be passed multiple times?

@bp7968h
Copy link
Contributor Author

bp7968h commented Nov 16, 2024

Hey @bp7968h, we're ready to merge this PR, could you please resolve the conflicts?

@bp7968h could you please open a PR to deno-docs repo to update this file: https://github.com/denoland/docs/blob/main/deploy/manual/environment-variables.md#deployment-environment-variables to mention that the flag can be passed multiple times?

Hi @bartlomieju, is that the only place where the docs should be updated, but when I google deno env I get the following link https://docs.deno.com/runtime/reference/env_variables/, should I update the docs here as well, need you suggestions please.

@bartlomieju
Copy link
Member

Sure, that's a good idea to update that page too.

@bp7968h
Copy link
Contributor Author

bp7968h commented Nov 17, 2024

Sure, that's a good idea to update that page too.

Hi @bartlomieju, I have created the PR, can you please review if the writing is ok.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @bp7968h, nice work

@bartlomieju bartlomieju enabled auto-merge (squash) November 17, 2024 22:12
@bartlomieju bartlomieju merged commit cff6e28 into denoland:main Nov 17, 2024
17 checks passed
@bp7968h bp7968h deleted the support-multiple-env-file-26425 branch November 21, 2024 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--env-file should be accepted multiple times for retro compatibility with nodejs
4 participants