feat(unstable): `deno run --env` #20300

iuioiua · 2023-08-27T00:44:34Z

This change adds the --env=[FILE] flag to the run, compile, eval, install and repl subcommands. Environment variables set in the CLI overwrite those defined in the .env file.

bartlomieju

Good start 👍 can you add some integration tests that check that the file is actually loaded?

cli/args/flags.rs

cli/tools/run.rs

cknight · 2023-08-27T10:35:58Z

How does this change fit in with the dotenv std module? There is obviously overlap, loading from a .env file. Some thoughts:

Is this needed if all this functionality and more can be achieved with a single line of code (import of std/dotenv/load.ts)?
Is it confusing having this functionality available in both CLI flag and std module?
Conversely, with a .env.example and .env.defaults files and the ability to access the key/value pairs without exporting to the process environment there is an argument for keeping the std dotenv module as it provides added value which dotenvy doesn't offer
Do dotenvy and std/dotenv parse a .env file in the same way with the same rules and capabilities? (Does it matter?)
I do like the explicit command line reference to loading a .env file, this makes it more explicit that you are dependent on env variables

bartlomieju · 2023-08-27T10:57:28Z

How does this change fit in with the dotenv std module? There is obviously overlap, loading from a .env file.
Some thoughts:

Is this needed if all this functionality and more can be achieved with a single line of code (import of std/dotenv/load.ts)?

There is some overlap, however with std/dotenv you can't influence any of DENO_* env vars without spawning a subprocess, while with --env flag these env variables will be applied before Deno checks for values of these env vars.

Is it confusing having this functionality available in both CLI flag and std module?

Probably yes, however --env is way more convenient than having to import module from the standard library, plus see previous comment.

Conversely, with a .env.example and .env.defaults files and the ability to access the key/value pairs without exporting to the process environment there is an argument for keeping the std dotenv module as it provides added value which dotenvy doesn't offer

We would certainly keep the std/dotenv module for people who need more control or flexibility. The --env flag is meant to serve the 80-90% use case.

Do dotenvy and std/dotenv parse a .env file in the same way with the same rules and capabilities? (Does it matter?)

Not sure, and I'm not sure if that matters. We will need to check that. It's not 100% agreed that we'll use dotenvy crate - AFAIK it can't do variable substitution which is one of the requirements for the --env flag. We already have some infrastructure for dealing with env vars in deno_task_shell and we might go with fork/inspired implementation there instead of using a 3rd part crate.

I do like the explicit command line reference to loading a .env file, this makes it more explicit that you are dependent on env variables

That was also one of the requirements for this feature - no magic discovery, not walking up the file system. In more detail the requirements for this feature are as follows:

require --env flag, don't traverse up the CWD if the file is not found
multiline variables support
variable substitution
conflict handling - this is the least specified, I think we just need to decide if the conflicting env var should raise an error or overrwrite (or make it configurable via a flag)

cknight · 2023-08-27T16:54:02Z

Thanks @bartlomieju, makes sense.

iuioiua · 2023-08-28T08:34:07Z

Changes made. Still, work to do. Regarding the requirements:

Don't traverse up the CWD - does this just mean throw when an env file with a path above the CWD is defined?
Multiline support - not working with dotenvy for some reason
Variable substitution - need to test, dotenvy should do that automatically
Conflict handling - let me see if dotenvy does that automatically

TL;DR dotenvy should take care of the last 3 requirements automatically, but I need to confirm. Do we want to use deno_task_shell? If so, able to provide direction?

bartlomieju · 2023-08-28T08:44:22Z

Changes made. Still, work to do. Regarding the requirements:

Don't traverse up the CWD - does this just mean throw when an env file with a path above the CWD is defined?

No, we should return an error if the value of the --env flag can't be loaded.

Multiline support - not working with dotenvy for some reason

That's a bummer :/

Variable substitution - need to test, dotenvy should do that automatically

That's good to hear 👍

Conflict handling - let me see if dotenvy does that automatically

👍

TL;DR dotenvy should take care of the last 3 requirements automatically, but I need to confirm. Do we want to use deno_task_shell? If so, able to provide direction?

If that's the case we should consider dotenvy for this purpose. If not, we can land the first pass as is and we can update deno_task_shell to be able to to handle these. Let's leave it for a follow up.

We also floated the idea that env vars defined in the dotenv file wouldn't require --allow-env permission to access, but that involves some more changes. @nayeemrmn thoughts on this?

nayeemrmn · 2023-08-28T14:02:37Z

We also floated the idea that env vars defined in the dotenv file wouldn't require --allow-env permission to access

Couldn't there be secrets there? Like API keys to non-production resources.

bartlomieju · 2023-08-29T08:24:47Z

Good point. I guess we can skip that part for now and reevaluate in the future.

bartlomieju · 2023-08-30T13:46:36Z

@iuioiua did you have a chance to verify if variable substitution and conflict handling is working in dotenvy?

iuioiua · 2023-08-30T17:24:39Z

Not yet. I should have a chance within the next couple of days. That cool?

iuioiua · 2023-08-31T01:53:26Z

Conflict handling - let me see if dotenvy does that automatically

dotenvy doesn't throw on conflicting environment variables. Instead, only the first value is used. I think this behaviour is fine, as long as it's explained, which I included. WDYT?

cknight · 2023-08-31T08:36:13Z

I think the more common use case would be if the env variable set in .env was already in the process environment, rather than the use case of duplication in the .env file. I think the behaviour is the same (the process env value is not overwritten), but it might be worth explicitly stating this in the docs.

dsherret · 2023-09-18T14:13:40Z

cli/args/flags.rs

@@ -391,6 +391,7 @@ pub struct Flags {
  pub ext: Option<String>,
  pub ignore: Vec<PathBuf>,
  pub import_map_path: Option<String>,
+  pub env_file: Option<String>,


That was also one of the requirements for this feature - no magic discovery, not walking up the file system.

Generally a .env file would be stored right beside a deno.json. If a deno.json exists, maybe we should traverse up and stop at the deno.json directory when someone provides --env with no path?

dsherret · 2023-09-18T15:38:26Z

I looked into this a bit and started extracting out some of the code from deno_task_shell, which will help us get this feature exactly how we like it and improve future integration. It's not too complicated, but there are some differences between the variable expansion (see https://github.com/motdotla/dotenv-expand/blob/master/tests/.env). Let's push this off to the next release.

iuioiua · 2023-09-28T08:41:03Z

I think the more common use case would be if the env variable set in .env was already in the process environment, rather than the use case of duplication in the .env file. I think the behaviour is the same (the process env value is not overwritten), but it might be worth explicitly stating this in the docs.

I've updated the command description.

It's not too complicated, but there are some differences between the variable expansion (see https://github.com/motdotla/dotenv-expand/blob/master/tests/.env).

I've updated tests to match those, which are expectedly now failing. Is that what you'd like us to aim towards, @dsherret?

I looked into this a bit and started extracting out some of the code from deno_task_shell, which will help us get this feature exactly how we like it and improve future integration.

Can you elaborate on how this is done?

dsherret · 2023-10-03T16:40:33Z

I looked into this a bit and started extracting out some of the code from deno_task_shell, which will help us get this feature exactly how we like it and improve future integration.

Can you elaborate on how this is done?

Extracted out some of the parsing functions and repurposed them. I haven't worked on it for a few weeks and it currently doesn't compile.

I've updated tests to match those, which are expectedly now failing. Is that what you'd like us to aim towards, @dsherret?

I think we can just merge this with dotenvy as a first pass.

Signed-off-by: Asher Gomez <ashersaupingomez@gmail.com>

iuioiua · 2023-10-31T23:31:34Z

cli/args/mod.rs

+    if let Some(env_file_name) = &flags.env_file {
+      from_filename(env_file_name).unwrap_or_else(|_| {
+        panic!(
+          "Unable to load '{}' environment variable file",
+          env_file_name.as_str()
+        )
+      });
+    }
+


Panicking seems wrong to me. How would we like to handle the error?

Use bail! instead which will return an error

iuioiua · 2023-11-01T11:55:33Z

Alright, I think this is ready. I've completed up to what I can, for now. I'm unsure how to do the present and missing env file tests for the compile and install sub-commands or if they're even needed. If they are needed, please go ahead and make any changes necessary. Hopefully, we can land this for the next release.

dsherret

LGTM. Thanks @iuioiua!

Let's land this as unstable then circle back with other fixes and improvements in the coming releases.

This reverts commit 117bfc9.

iuioiua · 2023-11-01T18:47:59Z

Nice! Thanks for making the finishing touches. Sorry it took so long 😅

iuioiua added 3 commits August 27, 2023 10:36

feat: --env-file

ecbea71

works

6107636

Merge branch 'main' into env-file

4399429

iuioiua marked this pull request as ready for review August 27, 2023 06:41

iuioiua added 3 commits August 27, 2023 16:44

fmt

c726df4

fix

4e5313b

fix

660e8ea

bartlomieju reviewed Aug 27, 2023

View reviewed changes

cli/args/flags.rs Outdated Show resolved Hide resolved

cli/args/flags.rs Outdated Show resolved Hide resolved

cli/tools/run.rs Outdated Show resolved Hide resolved

cli/tools/run.rs Outdated Show resolved Hide resolved

bartlomieju added this to the 1.37 milestone Aug 27, 2023

iuioiua changed the title ~~feat: --env-file~~ feat: --env Aug 28, 2023

better

1beb517

iuioiua requested a review from bartlomieju August 28, 2023 08:34

fix

108ad01

work

a28fbc6

iuioiua added 2 commits August 31, 2023 11:54

tweak

932ade7

Merge branch 'main' into env-file

b1e717a

cknight mentioned this pull request Sep 1, 2023

Feedback on proposal: Deprecate std/dotenv denoland/std#3604

Closed

Merge branch 'main' into env-file

9f2995f

dsherret reviewed Sep 18, 2023

View reviewed changes

dsherret modified the milestones: 1.37, 1.38 Sep 18, 2023

dsherret mentioned this pull request Sep 27, 2023

Environment Variables in Test #20717

Closed

iuioiua added 2 commits September 28, 2023 17:17

Merge branch 'main' into env-file

8e634d3

refactor: test and command description

301ed69

iuioiua changed the title ~~feat: --env~~ feat: deno <run> --env Sep 28, 2023

iuioiua changed the title ~~feat: deno <run> --env~~ feat: deno run --env Sep 28, 2023

iuioiua added 5 commits October 10, 2023 11:36

Merge branch 'main' into env-file

20ada36

fix

eb67bdf

work

91fd73e

fix

6d61c34

Merge branch 'main' into env-file

cab593e

Signed-off-by: Asher Gomez <ashersaupingomez@gmail.com>

iuioiua commented Oct 31, 2023

View reviewed changes

iuioiua added 2 commits November 1, 2023 12:14

work

dbf1775

work

2cc675c

Add UNSTABLE to doc

c942eb2

dsherret changed the title ~~feat: deno run --env~~ feat(unstable): deno run --env Nov 1, 2023

dsherret approved these changes Nov 1, 2023

View reviewed changes

Surface underlying error

117bfc9

dsherret enabled auto-merge (squash) November 1, 2023 14:05

Revert "Surface underlying error"

b0204c0

This reverts commit 117bfc9.

dsherret merged commit f8f4e77 into denoland:main Nov 1, 2023

iuioiua deleted the env-file branch November 1, 2023 18:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(unstable): `deno run --env` #20300

feat(unstable): `deno run --env` #20300

iuioiua commented Aug 27, 2023 •

edited

Loading

bartlomieju left a comment

cknight commented Aug 27, 2023

bartlomieju commented Aug 27, 2023 •

edited

Loading

cknight commented Aug 27, 2023

iuioiua commented Aug 28, 2023 •

edited

Loading

bartlomieju commented Aug 28, 2023

nayeemrmn commented Aug 28, 2023

bartlomieju commented Aug 29, 2023

bartlomieju commented Aug 30, 2023

iuioiua commented Aug 30, 2023

iuioiua commented Aug 31, 2023

cknight commented Aug 31, 2023

dsherret Sep 18, 2023

dsherret commented Sep 18, 2023

iuioiua commented Sep 28, 2023

dsherret commented Oct 3, 2023

iuioiua Oct 31, 2023 •

edited

Loading

bartlomieju Nov 1, 2023

iuioiua commented Nov 1, 2023

dsherret left a comment

iuioiua commented Nov 1, 2023

feat(unstable): deno run --env #20300

feat(unstable): deno run --env #20300

Conversation

iuioiua commented Aug 27, 2023 • edited Loading

bartlomieju left a comment

Choose a reason for hiding this comment

cknight commented Aug 27, 2023

bartlomieju commented Aug 27, 2023 • edited Loading

cknight commented Aug 27, 2023

iuioiua commented Aug 28, 2023 • edited Loading

bartlomieju commented Aug 28, 2023

nayeemrmn commented Aug 28, 2023

bartlomieju commented Aug 29, 2023

bartlomieju commented Aug 30, 2023

iuioiua commented Aug 30, 2023

iuioiua commented Aug 31, 2023

cknight commented Aug 31, 2023

dsherret Sep 18, 2023

Choose a reason for hiding this comment

dsherret commented Sep 18, 2023

iuioiua commented Sep 28, 2023

dsherret commented Oct 3, 2023

iuioiua Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

bartlomieju Nov 1, 2023

Choose a reason for hiding this comment

iuioiua commented Nov 1, 2023

dsherret left a comment

Choose a reason for hiding this comment

iuioiua commented Nov 1, 2023

feat(unstable): `deno run --env` #20300

feat(unstable): `deno run --env` #20300

iuioiua commented Aug 27, 2023 •

edited

Loading

bartlomieju commented Aug 27, 2023 •

edited

Loading

iuioiua commented Aug 28, 2023 •

edited

Loading

iuioiua Oct 31, 2023 •

edited

Loading