src: add support to override local env variables option #52531

IlyasShabi · 2024-04-14T18:45:04Z

Following #49148
This PR aims to add support to override local env variables with variables from defined env files.

using CLI
using loadEnvFile

nodejs-github-bot · 2024-04-14T18:45:09Z

Review requested:

@nodejs/startup

anonrig

I think I miss the whole point. What's the use case and the feature request that resulted in this change?

IlyasShabi · 2024-04-15T08:21:28Z

@anonrig when reading the issue about using .env files, I noticed a discussion about overriding local environment settings:
#49148 (comment)
#49148 (comment)
I created this PR to add support for overriding, similar to what dotenv offers here
Is there a specific process I should follow before creating a PR?

Trott · 2024-04-19T21:00:37Z

I'm not sure who to ping for more eyes/comments on this, but I'll try @nodejs/tooling.

doc/api/cli.md

src/node_dotenv.cc

anonrig · 2024-04-19T23:25:06Z

src/node_options.cc

@@ -627,6 +627,11 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
            "set environment variables from supplied file",
            &EnvironmentOptions::env_file);
  Implies("--env-file", "[has_env_file_string]");
+  AddOption("--override-env-var",
+            "override environment variables set on machine with variables from "
+            "supplied file",


there needs to be a description with the usage of env-file flag or loadEnvFile

@anonrig could you check if it's correct please ?

test/parallel/test-process-load-env-file.js

doc/api/cli.md

mcollina

Lgtm

GeoffreyBooth · 2024-04-20T17:14:05Z

Is there a specific process I should follow before creating a PR?

There isn’t, but generally I encourage people to open an issue proposing a new feature before going ahead and coding the new feature. That way you get preliminary feedback before you implement, which might change the way you implement it and save you time as compared to doing revisions (like renaming this flag, for example). Or if the feedback is very negative, it might save you from wasting your time implementing a feature that won’t land. If nothing else, it separates the feedback related to the idea of the feature from the feedback related to the implementation of the feature; the issue can contain the former and the PR would get the latter.

For this feature in particular, since it’s not on the list of anticipated enhancements to --env-file, I think it would have been good to open an issue dedicated to this to discuss specifically whether this is something that we want. My initial impression is that if it’s something that we thought would be good to add, it would’ve been on the TODO list; so I’m already skeptical as to whether this is a good idea or not. I don’t think the question has been asked directly, though, so maybe it’s fine; I think it’s worth having that conversation, in more detail than the few comments you found within other threads. I think in general we’re not trying to put dotenv out of business; our scope is not to match every feature they offer, so just because dotenv does something doesn’t mean that we’ll automatically copy it.

IlyasShabi · 2024-04-20T22:23:03Z

@GeoffreyBooth Thank you for explaining this. I was inspired by the discussions to contribute.
I see why it's important to discuss new features in an issue first. This can help shape the feature better and save time.
In the future, I'll start with an issue to gather feedback before I begin coding.

src/node_process_methods.cc

doc/api/process.md

test/parallel/test-process-load-env-file.js

anonrig

Small things left on docs and tests. After them I think we can merge this.

anonrig · 2024-04-21T22:56:30Z

doc/api/cli.md

+
+If you want to use the value from the `.env` file, you should add the
+`--env-file-override-local` flag when running your application, or use
+[`process.loadEnvFile({ override: true })`][] in your code.


This line is unnecessary for the CLI flag documentation. You should also mention how --env-file flag is required as well

anonrig · 2024-04-21T22:57:25Z

doc/api/process.md

+```cjs
+const assert = require('node:assert');
+const { loadEnvFile } = require('node:process');
+loadEnvFile('.env', { override: true });


Process is a global variable. No need to require/import it.

Suggested change

loadEnvFile('.env', { override: true });

process.loadEnvFile('.env', { override: true });

I was forced to import it because of the linter no-restricted-globals.
I will disable the eslint rule then

anonrig · 2024-04-21T22:59:39Z

test/parallel/test-process-load-env-file.js

+  });
+
+  it('supports passing options only', async () => {
+    process.env.BASIC = 'Original value';


If you pass this variable in here, you mutate the current process which gets inherited by the spawn process call. But if you pass { env } as an option, it would not mutate the current process env vars.

github-actions · 2024-04-21T23:00:21Z

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @anonrig.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

bnb · 2024-04-22T20:31:57Z

doc/api/process.md

@@ -2305,6 +2313,52 @@ import { loadEnvFile } from 'node:process';
 loadEnvFile();
 ```

+Override local environment variables using override option
+
+Example:


I believe this line is unnecessary, it's not a pattern that exists anywhere else in the docs AFAIK

This sentence appears multiple times in the doc:

https://nodejs.org/api/cli.html#--allow-worker
https://nodejs.org/api/fs.html#fs-constants
https://nodejs.org/api/http.html#httpvalidateheadernamename-label
and more.

If you think this is not a good practice I can remove it.

bnb

currently these examples use a very mixed bag of import/require. You import/require assert, sometimes process, and sometimes import process... and a named export of process, and sometimes just a named export of process.

I've never seen mixing the module and a named import, maybe it's just a pattern I'm ignorant of? Either way, IMO we should just be consistent across all examples - named exports are the most direct avenue to that, I think.

Blocking because I'd really like to try to not land docs that are less than ideal and then fix later. Should be a pretty fast tweak, happy to unblock once it's addressed.

joyeecheung · 2024-04-23T15:05:39Z

currently these examples use a very mixed bag of import/require.

From what I can tell the docs are just using

direct access of process from the context + require('node:assert') in CJS examples
import 'node:process' + import 'node:assert' in ESM examples

I am not sure if you noticed but that has been the way to write the examples in both ways to allow toggling between CJS/ESM style in the doc (e.g. see https://nodejs.org/docs/latest/api/fs.html#promise-example for how the toggling works in the web page).

bnb · 2024-04-23T19:09:27Z

@joyeecheung yep, I'm aware - I've written a handful of docs that do that/added missing CJS or ESM to examples where only one is present.

Here's examples of how the docs here are inconsistent:

import assert from 'node:assert';
import process from 'node:process';

const assert = require('node:assert');
const { loadEnvFile } = require('node:process');
loadEnvFile('.env', { override: true });

import assert from 'node:assert';
import process, { loadEnvFile } from 'node:process';
loadEnvFile('.env', { override: true });
assert.strictEqual(process.env.API_KEY, 'env-custom-key');

import'ing process and then { loadEnvFile } is IMO a very weird pattern? I've never seen it before at least. The flipping between implicit process vs the one in ESM kinda is awful ngl - is this required? I guess I didn't know that was how you'd do this.

There's kinda a lot in a small space, so I figured pointing issues out broadly as the few categories that occur multiple times would be helpful but if it's not I'm happy to comment every problem individually.

joyeecheung · 2024-04-24T03:57:39Z

I see, there are some inconsistencies in import styles though I think in this code base in general, style preferences should not be blocking unless the linter complains. I’d suggest adding linter rules to forbid it or following it up with doc change PR.

bnb

disagree that we shouldn't block on docs being inconsistent but I don't think this is the venue to hash that out

GeoffreyBooth · 2024-04-28T17:35:37Z

disagree that we shouldn’t block on docs being inconsistent but I don’t think this is the venue to hash that out

I don’t think it’s a blocking concern for this PR. It’s something that can be handled in a follow-up that focuses just on standardizing the code examples in the docs, ideally with a lint rule to enforce future consistency.

IlyasShabi · 2024-04-30T09:07:34Z

Do you think this PR is ready to be merged, or does it still need additional work?

anonrig · 2024-05-07T00:01:10Z

Do you think this PR is ready to be merged, or does it still need additional work?

I'm sorry I missed this. I think we can merge it.

nodejs-github-bot · 2024-05-07T00:25:32Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58998/

IlyasShabi · 2024-05-14T20:06:00Z

@anonrig Could you please run the CI ?

aduh95 · 2024-05-17T10:37:21Z

doc/api/cli.md

+Assume you have a local environment variable on your machine
+
+```bash
+API_KEY="local-key"
+```
+
+Meanwhile, your `.env` file contains:
+
+```bash
+API_KEY='env-custom-key'
+```
+
+If you want to use the value from the `.env` file, you should add the
+`--env-file-override-local` flag when running your application.
+This flag requires the use of the `--env-file` flag to load environment files.
+
+```js
+console.log(process.env.API_KEY); // 'env-custom-key'
+```
+
+If you prefer to keep the local value, do not add the
+`--env-file-override-local` flag or avoid providing the override option to
+[`process.loadEnvFile()`][]
+
+```js
+console.log(process.env.API_KEY); // 'local-key'
+```


I think the example should go more straight to the point

Suggested change

Assume you have a local environment variable on your machine

```bash

API_KEY="local-key"

```

Meanwhile, your `.env` file contains:

```bash

API_KEY='env-custom-key'

```

If you want to use the value from the `.env` file, you should add the

`--env-file-override-local` flag when running your application.

This flag requires the use of the `--env-file` flag to load environment files.

```js

console.log(process.env.API_KEY); // 'env-custom-key'

```

If you prefer to keep the local value, do not add the

`--env-file-override-local` flag or avoid providing the override option to

[`process.loadEnvFile()`][]

```js

console.log(process.env.API_KEY); // 'local-key'

```

```console

$ echo 'API_KEY="local-key"' > .env

$ node --env-file=.env -p 'process.env.API_KEY'

local-key

$ export API_KEY='env-custom-key'

$ node --env-file=.env -p 'process.env.API_KEY'

env-custom-key

$ node --env-file=.env --env-file-override-local -p 'process.env.API_KEY'

local-key

```

aduh95 · 2024-05-17T10:41:39Z

doc/api/process.md

+Assume you have a local environment variable on your machine
+
+```bash
+API_KEY="local-key"
 ```

+Meanwhile, your `.env` file contains:
+
+```bash
+API_KEY='env-custom-key'
+```
+
+If you want to use the value from the `.env` file
+
+```js
+process.loadEnvFile('.env', { override: true });
+console.log(process.env.API_KEY); // 'env-custom-key'
+```
+
+Load environment variables with options only
+
+```js
+process.loadEnvFile({ override: true });
+```
+
+This API is available through the [`--env-file-override-local`][] flag.


Same here

Suggested change

Assume you have a local environment variable on your machine

```bash

API_KEY="local-key"

```

Meanwhile, your `.env` file contains:

```bash

API_KEY='env-custom-key'

```

If you want to use the value from the `.env` file

```js

process.loadEnvFile('.env', { override: true });

console.log(process.env.API_KEY); // 'env-custom-key'

```

Load environment variables with options only

```js

process.loadEnvFile({ override: true });

```

This API is available through the [`--env-file-override-local`][] flag.

```console

$ echo 'API_KEY="local-key"' > .env

$ node -p 'process.loadEnvFile('.env');process.env.API_KEY'

local-key

$ export API_KEY='env-custom-key'

$ node -p 'process.loadEnvFile('.env');process.env.API_KEY'

env-custom-key

$ node -p 'process.loadEnvFile('.env', { override: true });process.env.API_KEY'

local-key

$ node --env-file-override-local -p 'process.loadEnvFile('.env');process.env.API_KEY'

env-custom-key

```

doc/api/process.md

lib/internal/process/per_thread.js

aduh95 · 2024-05-18T05:43:59Z

src/node_options.cc

+      "override environment variables set on machine with variables from "
+      "supplied files via --env-file flag or loadEnvFile() process function",


Is that correct that the flag has influence over loadEnvFile()? Could we add a test case for that?

Suggested change

"override environment variables set on machine with variables from "

"supplied files via --env-file flag or loadEnvFile() process function",

"override environment variables set on machine with variables from "

"supplied files via --env-file flag",

nodejs-github-bot · 2024-06-09T14:58:40Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59710/

GeoffreyBooth · 2024-06-10T00:52:59Z

What if #53060 lands? And someone passes both --env-file and --env-file-optional. Does this flag affect both? Would there be any way to specify “override local variables for one .env file, but not for the other”? (Or do we need such an ability?)

BoscoDomingo · 2024-06-11T09:44:12Z

What if #53060 lands? And someone passes both --env-file and --env-file-optional. Does this flag affect both? Would there be any way to specify “override local variables for one .env file, but not for the other”? (Or do we need such an ability?)

I'd argue for the former: it's simpler, more consistent (if you pass multiple --env-file this flag already affects them all) and at that point users are trying to do a fairly complex operation for which a specialised tool/script may be more suitable, but open to feedback

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Apr 14, 2024

IlyasShabi force-pushed the dotenv-override branch from 689209b to 20a9ddc Compare April 14, 2024 18:45

IlyasShabi marked this pull request as ready for review April 14, 2024 18:50

anonrig reviewed Apr 14, 2024

View reviewed changes

anonrig reviewed Apr 19, 2024

View reviewed changes

anonrig requested review from GeoffreyBooth, joyeecheung and mcollina April 19, 2024 23:46

mcollina approved these changes Apr 20, 2024

View reviewed changes

IlyasShabi force-pushed the dotenv-override branch from 20a9ddc to 3607629 Compare April 20, 2024 22:14

anonrig reviewed Apr 20, 2024

View reviewed changes

src/node_process_methods.cc Outdated Show resolved Hide resolved

anonrig reviewed Apr 20, 2024

View reviewed changes

src/node_process_methods.cc Outdated Show resolved Hide resolved

anonrig reviewed Apr 20, 2024

View reviewed changes

anonrig reviewed Apr 21, 2024

View reviewed changes

anonrig added semver-minor PRs that contain new features and should be released in the next minor version. notable-change PRs with changes that should be highlighted in changelogs. labels Apr 21, 2024

bnb reviewed Apr 22, 2024

View reviewed changes

bnb requested changes Apr 22, 2024

View reviewed changes

IlyasShabi force-pushed the dotenv-override branch from 8f4f8c2 to 5328e49 Compare April 26, 2024 21:20

IlyasShabi force-pushed the dotenv-override branch 3 times, most recently from c39b038 to ec91591 Compare April 27, 2024 21:39

bnb approved these changes Apr 28, 2024

View reviewed changes

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label May 7, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 7, 2024

IlyasShabi force-pushed the dotenv-override branch 2 times, most recently from de5019c to 304248e Compare May 14, 2024 14:03

anonrig approved these changes May 14, 2024

View reviewed changes

src: add support to override local env variables option

6c03fd4

IlyasShabi force-pushed the dotenv-override branch from 304248e to c723fb0 Compare May 17, 2024 07:47

aduh95 reviewed May 17, 2024

View reviewed changes

doc/api/process.md Outdated Show resolved Hide resolved

aduh95 reviewed May 17, 2024

View reviewed changes

lib/internal/process/per_thread.js Outdated Show resolved Hide resolved

src: fix tests and update doc

a13b1a2

IlyasShabi force-pushed the dotenv-override branch from c723fb0 to a13b1a2 Compare May 17, 2024 22:02

aduh95 reviewed May 18, 2024

View reviewed changes

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 9, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 9, 2024

	loadEnvFile('.env', { override: true });
	process.loadEnvFile('.env', { override: true });

		"override environment variables set on machine with variables from "
		"supplied files via --env-file flag or loadEnvFile() process function",

Uh oh!

src: add support to override local env variables option #52531

Are you sure you want to change the base?

src: add support to override local env variables option #52531

Uh oh!

Conversation

IlyasShabi commented Apr 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 14, 2024

Uh oh!

anonrig left a comment

Choose a reason for hiding this comment

Uh oh!

IlyasShabi commented Apr 15, 2024

Uh oh!

Trott commented Apr 19, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

GeoffreyBooth commented Apr 20, 2024

Uh oh!

IlyasShabi commented Apr 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anonrig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnb commented Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Apr 24, 2024

Uh oh!

bnb left a comment

Choose a reason for hiding this comment

Uh oh!

GeoffreyBooth commented Apr 28, 2024

Uh oh!

IlyasShabi commented Apr 30, 2024

Uh oh!

anonrig commented May 7, 2024

Uh oh!

nodejs-github-bot commented May 7, 2024

IlyasShabi commented Apr 14, 2024 •

edited

Loading

IlyasShabi commented Apr 20, 2024 •

edited

Loading

bnb left a comment •

edited

Loading

joyeecheung commented Apr 23, 2024 •

edited

Loading

bnb commented Apr 23, 2024 •

edited

Loading

aduh95 May 17, 2024 •

edited

Loading

aduh95 May 17, 2024 •

edited

Loading

aduh95 May 18, 2024 •

edited

Loading