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

src: don't overwrite environment from .env file #49424

Conversation

philnash
Copy link
Contributor

@philnash philnash commented Sep 1, 2023

This commit adds a check to see if an environment variable that is found in the .env file is already set in the environment. If it is, then the value from the .env file is not used.

The docs from #48890 say:

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

In my testing, I found that not to be the case, so I went to look for a solution.

Caveats. I am not a C++ programmer and have never tried to read, understand or contribute back to a C++ code base before. This is a naive solution. Please feel free to criticize and correct my C++.

I know the v20.6.0 release is very close, but as dotenv support is a notable feature, I think this should join the release so that we don't push a feature that contradicts its own documentation.

This commit adds a check to see if an environment variable that is
found in the .env file is already set in the environment. If it is,
then the value from the .env file is not used.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Sep 1, 2023
@anonrig anonrig self-requested a review September 1, 2023 11:52
@ghost
Copy link

ghost commented Sep 1, 2023

This seems to contradict what the average expectation would be when using the --env-file option. If I ran something with the option and then logged different values from what was in my .env file, I would be extremely confused or think something is broken. A very common case of using an environment file would be to overwrite/ignore any existing variables with the same name.

On a side note, can you link to where you found that quote? I couldn't find that anywhere in the linked thread. The closest I saw was an incomplete action item that said needed more discussion.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 1, 2023

On a side note, can you link to where you found that quote?

https://github.com/nodejs/node/pull/48890/files#diff-7349c17bce4d97526eefcb6f8291d617e7edb92ed9927fd3b9264e8014fc4e7cR998-R999

I agree with @TacoZilla here. I think we should change the docs, not the code. More locally defined config should take precedence over less locally defined config.

@philnash
Copy link
Contributor Author

philnash commented Sep 1, 2023

All the existing implementations of dotenv behave this way. You can see that the Node.js dotenv library has an override option that is false by default. The Ruby library has an overload method, but won't do by it by default. The Python dotenv behaves this way too.

I think this feature should follow the prior implementations.

Though we should also consider a way to override the environment for a future release too.

@anonrig
Copy link
Member

anonrig commented Sep 1, 2023

I agree with @philnash. I didn't had any time to check the code but I think this is the correct path.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 1, 2023

TIL. I agree we should follow what all of the other implementations do.

Side note: It seems like a design decision with obvious downsides. I'm curious if there are advantages I'm just missing to overriding by default other than the consistency aspect.

@ghost
Copy link

ghost commented Sep 1, 2023

The significant difference to me is that libraries like dotenv are used/required/imported during runtime in application code. This feature is a CLI option you pass to the node executable. While I understand their end goals are similar (e.g. expose environment variables to your application), I'd argue the implementation matters.

If node was offering a module that you used similarly to dotenv, I'd be fine matching the existing behavior, but it feels wrong in the current implementation. This conflicts (albeit maybe not in the conventional way of thinking) with the precedent that CLI options always override environment variables:

Options from the command line take precedence over options passed through the NODE_OPTIONS environment variable.

On one hand, you could make the argument this isn't actually violating that because the option is always used even if the end result is a no-op, but on the other hand, I'd argue it does violate things in spirit. If I pass a CLI option, I expect it to have highest priority / override anything previously set, but it's entirely possible I'm just weird :P

There should definitely be some way to override existing environment variables, so I think everyone at least agrees on that in some form.

@philnash
Copy link
Contributor Author

philnash commented Sep 2, 2023

Honestly, I'm sort of torn here. I've never really liked the default way that any dotenv implementation works, I expect my local settings to override the environment. I've been caught out by that many times before and I don't know why that was the original decision that was taken.

However, as I've said above, it is the default for many, if not all, implementations of dotenv. If Node is to provide an implementation of dotenv, even a simple one compared to the options that are out there, then I think it should match what already exists. I think that if someone who is using dotenv-node with default settings decides to drop it in place of using --env-file then it should work the same for them.

And that's why I wrote this pull request. It goes against how I think the feature should work, but it follows how everyone who does know the feature already knows how it works.

(And yes, we should add override as an option somehow, but that can be done in a further PR.)

@anonrig anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. review wanted PRs that need reviews. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 2, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 2, 2023
@nodejs-github-bot
Copy link
Collaborator

@rluvaton
Copy link
Member

rluvaton commented Sep 2, 2023

Important note:

I think we need to make an exception for NODE_OPTIONS:

# .env
NODE_OPTIONS='--max-old-space-size=3096'
$ NODE_OPTIONS='--env-file=./.env' node something.js

I think we should override the NODE_OPTIONS

@anonrig
Copy link
Member

anonrig commented Sep 2, 2023

I think we should override the NODE_OPTIONS

NODE_OPTIONS has already an exception (defined at 769823e#diff-6c1386cbc6e4a11941649af31a0257d5c3e40b5f41223cdddf87ab2e984b705aR851), but you're right that we need to also make an exception in here, since we want to get the correct value for process.env.NODE_OPTIONS

src/node_dotenv.cc Show resolved Hide resolved
@GeoffreyBooth GeoffreyBooth added the cli Issues and PRs related to the Node.js command line interface. label Sep 2, 2023
@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Sep 2, 2023

I think we need to make an exception for NODE_OPTIONS:

I disagree, I think NODE_OPTIONS needs to behave like any other environment variable. It would be very confusing otherwise.

Yes we need to prevent an infinite loop, but that should be the only exceptional handling that we do.

NODE_OPTIONS has already an exception (defined at 769823e#diff-6c1386cbc6e4a11941649af31a0257d5c3e40b5f41223cdddf87ab2e984b705aR851)

My reading of this is that NODE_OPTIONS was behaving “correctly” (as in, the environment takes precedence over the file) because we carved out an exception to do exactly that for this one variable; but all other variables were behaving incorrectly. So it seems like we just need to make this exception the rule for all environment variables.

@GeoffreyBooth
Copy link
Member

Honestly, I’m sort of torn here. I’ve never really liked the default way that any dotenv implementation works, I expect my local settings to override the environment. I’ve been caught out by that many times before and I don’t know why that was the original decision that was taken.

I understand the sentiment. I think you can achieve your desired behavior, at least in Bash and similar shells, via:

set -a; source .env; set +a; node app.js

Which is a bit clunky but gets the job done. Maybe this is why the default is the way it is in those other libraries.

@ghost
Copy link

ghost commented Sep 2, 2023

I understand the sentiment. I think you can achieve your desired behavior, at least in Bash and similar shells, via:

set -a; source .env; set +a; node app.js

Which is a bit clunky but gets the job done. Maybe this is why the default is the way it is in those other libraries.

Isn't that argument somewhat self-defeating? Using the same argument, you could ask why node is implementing this as an option in the first place?

With that being said, I think framing this as should node match dotenv is missing the point. A better question is why dotenv implementations behave the way they do and then ask if that same logic holds true for this case.

From the Ruby implementation's docs:

By default, it won't overwrite existing environment variables as dotenv assumes the deployment environment has more knowledge about configuration than the application does.

I would argue that reasoning doesn't hold true for node's implementation. Passing a CLI option to the node executable would live pretty firmly in the context of the deployment environment, as such, it should take precedence.

At the end of the day, if node's priority is simply to blindly match dotenv as closely as possible, then this entire discussion is moot. I don't agree with blindly matching for the sake of matching, but I do understand that line of reasoning, but in that case I think we should be very clear and acknowledge it's not necessarily the actual best way to implement it.

On a slight tangent, I really don't understand when this default behavior would be useful in the average case. If a user is explicitly passing in an environment file, the chances of then wanting already set environment variables to take precedence would be very low if not directly contradict using the CLI option to begin with.

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Sep 2, 2023

I would argue that reasoning doesn’t hold true for node’s implementation. Passing a CLI option to the node executable would live pretty firmly in the context of the deployment environment, as such, it should take precedence.

In the initial discussions for the feature we had discussed making loading .env happen automatically, as in node --env-file=.env would be the default behavior and wouldn’t need a flag. This would be a semver-major change, so we added the flag instead as a way to opt-in; the value for the flag, to load a file with a name other than .env, was a later addition. We might still want to auto-load .env files in a future major version of Node, in which case this “because it’s via CLI option” argument no longer holds.

That doesn’t mean we couldn’t have one precedence behavior for autoloaded files versus CLI-referenced files, but I’m assuming that we wouldn’t want such a divergence in behavior.

Isn’t that argument somewhat self-defeating? Using the same argument, you could ask why node is implementing this as an option in the first place?

Because there’s a difference between --env-file and set -a; source .env; set +a; node: the former gives variables already present in the environment precedence, the latter gives variables defined in the file precedence.

By default, it won’t overwrite existing environment variables as dotenv assumes the deployment environment has more knowledge about configuration than the application does.

What the Ruby docs describe is exactly how I’d use this feature myself. I can imagine having a .env file committed into the repo with contents like:

ROOT_URL=http://localhost:3000
DATABASE_USER=postgres
DATABASE_PASS=postgres

And these obviously wouldn’t be the values in the prod environment. I could have an npm start command of node --env-file=.env app.js and it can run everywhere without changes, because in production the ROOT_URL etc variables defined in that environment would override what’s in the file.

@GeoffreyBooth
Copy link
Member

One more point of comparison: Bun behaves like dotenv and Ruby and so on:

echo 'FOO=bar' > .env
echo 'console.log(process.env.FOO)' > test.js
bun test.js # bar
FOO=baz bun test.js # baz

@philnash philnash force-pushed the src-dont-overwrite-environment-from-dotenv branch from ae5cb39 to 271bebd Compare September 3, 2023 08:35
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 3, 2023
@philnash
Copy link
Contributor Author

philnash commented Sep 8, 2023

Another fail. This one is a problem

09:48:24         not ok 3 - TZ environment variable
09:48:24           ---
09:48:24           duration_ms: 99.603847
09:48:24           location: '/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos20-64/test/parallel/test-dotenv-node-options.js:47:3'
09:48:24           failureType: 'testCodeFailure'
09:48:24           error: |-
09:48:24             Expected values to be strictly equal:
09:48:24             + actual - expected
09:48:24             
09:48:24             + 'node:assert:399\n' +
09:48:24             +   '    throw err;\n' +
09:48:24             +   '    ^\n' +
09:48:24             +   '\n' +
09:48:24             +   'AssertionError [ERR_ASSERTION]: false == true\n' +
09:48:24             +   '    at [eval]:1:18\n' +
09:48:24             +   '    at Script.runInThisContext (node:vm:121:12)\n' +
09:48:24             +   '    at Object.runInThisContext (node:vm:295:38)\n' +
09:48:24             +   '    at node:internal/process/execution:83:21\n' +
09:48:24             +   '    at [eval]-wrapper:6:24\n' +
09:48:24             +   '    at runScript (node:internal/process/execution:82:62)\n' +
09:48:24             +   '    at evalScript (node:internal/process/execution:104:10)\n' +
09:48:24             +   '    at node:internal/main/eval_string:50:3 {\n' +
09:48:24             +   '  generatedMessage: true,\n' +
09:48:24             +   "  code: 'ERR_ASSERTION',\n" +
09:48:24             +   '  actual: false,\n' +
09:48:24             +   '  expected: true,\n' +
09:48:24             +   "  operator: '=='\n" +
09:48:24             +   '}\n' +
09:48:24             +   '\n' +
09:48:24             +   'Node.js v21.0.0-pre\n'
09:48:24             - ''
09:48:24           code: 'ERR_ASSERTION'
09:48:24           name: 'AssertionError'
09:48:24           expected: ''
09:48:24           actual: |-
09:48:24             node:assert:399
09:48:24                 throw err;
09:48:24                 ^
09:48:24             
09:48:24             AssertionError [ERR_ASSERTION]: false == true
09:48:24                 at [eval]:1:18
09:48:24                 at Script.runInThisContext (node:vm:121:12)
09:48:24                 at Object.runInThisContext (node:vm:295:38)
09:48:24                 at node:internal/process/execution:83:21
09:48:24                 at [eval]-wrapper:6:24
09:48:24                 at runScript (node:internal/process/execution:82:62)
09:48:24                 at evalScript (node:internal/process/execution:104:10)
09:48:24                 at node:internal/main/eval_string:50:3 {
09:48:24               generatedMessage: true,
09:48:24               code: 'ERR_ASSERTION',
09:48:24               actual: false,
09:48:24               expected: true,
09:48:24               operator: '=='
09:48:24             }
09:48:24             
09:48:24             Node.js v21.0.0-pre
09:48:24             
09:48:24           operator: 'strictEqual'
09:48:24           stack: |-
09:48:24             TestContext.<anonymous> (/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos20-64/test/parallel/test-dotenv-node-options.js:56:12)
09:48:24             process.processTicksAndRejections (node:internal/process/task_queues:95:5)
09:48:24             async Test.run (node:internal/test_runner/test:632:9)
09:48:24             async Suite.processPendingSubtests (node:internal/test_runner/test:374:7)
09:48:24           ...

In this case the environment set the TZ variable and the test expected to override it. I'm going to update the test to ensure that the variable is not part of the environment so that in all environments we can check that passing TZ in a .env file will correctly set the time zone for the Date class.

@anonrig
Copy link
Member

anonrig commented Sep 8, 2023

@nodejs/platform-aix @nodejs/platform-smartos Any idea why this is failing?

In some CI environments the TZ env var is set. Since a .env file won't
override that, we need to create an env where the test can still run by
deleting the TZ variable.
@philnash philnash force-pushed the src-dont-overwrite-environment-from-dotenv branch from 4565fa2 to 56f0868 Compare September 8, 2023 02:14
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 8, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 8, 2023
@nodejs-github-bot
Copy link
Collaborator

@philnash
Copy link
Contributor Author

philnash commented Sep 8, 2023

I think I'm getting the hang of analysing the Jenkins builds.

This fail seems to be the flaky IPv6 test again.

13:08:24 C:\workspace\node-compile-windows\node\test\cctest\test_inspector_socket.cc(323): error: Value of: (!expectation.read_expected)
13:08:24   Actual: true
13:08:24 Expected: false
13:08:24 
13:08:24 [  FAILED  ] InspectorSocketTest.HostIPv6NonRoutable (5000 ms)

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@philnash
Copy link
Contributor Author

philnash commented Sep 8, 2023

Live view of me hoping this time the CI will pass.

A man crying at a computer

I think one of the suites just straight failed to run this time.

@nodejs-github-bot
Copy link
Collaborator

@philnash
Copy link
Contributor Author

philnash commented Sep 8, 2023

All checks have passed

Celebrating with champagne

@atlowChemi atlowChemi added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 8, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 8, 2023
@nodejs-github-bot nodejs-github-bot merged commit 015e27b into nodejs:main Sep 8, 2023
27 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 015e27b

@bricss bricss mentioned this pull request Sep 8, 2023
@bricss bricss mentioned this pull request Sep 10, 2023
UlisesGascon pushed a commit that referenced this pull request Sep 11, 2023
This commit adds a check to see if an environment variable that is
found in the .env file is already set in the environment. If it is,
then the value from the .env file is not used.

PR-URL: #49424
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
UlisesGascon pushed a commit that referenced this pull request Sep 11, 2023
This commit adds a check to see if an environment variable that is
found in the .env file is already set in the environment. If it is,
then the value from the .env file is not used.

PR-URL: #49424
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
This commit adds a check to see if an environment variable that is
found in the .env file is already set in the environment. If it is,
then the value from the .env file is not used.

PR-URL: nodejs#49424
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. cli Issues and PRs related to the Node.js command line interface. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.