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: return error --env-file if file is missing #50588

Merged

Conversation

ardinugrxha
Copy link
Contributor

Motivated by #50536

NodeJS currently supports env file parsing. However, in cases where the file is missing, the process continues to execute. Since the env file is a crucial file, I propose that we return an error if the env file is missing. This will prevent errors in the future and allow us to detect them as early as possible.

Testing Scenario:

Given files tree:

└── test/
    ├── hello.js
    └── test.env

run: node --env-file=test.env hello.js
output: it will executed


└── test/
    ├── hello.js
    └── test.env

run: node --env-file=oops.env hello.js
output: node: oops.env not found


└── test/
    ├── hello.js
    └── test.env
    └── test1.env

run: node --env-file=test.env --env-file=test1.env hello.js
output: it will executed

└── test/
    ├── hello.js
    └── test.env
    └── test1.env

run: node --env-file=test.env --env-file=oops.env hello.js
output: node: oops.env not found

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@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 Nov 7, 2023
@ardinugrxha ardinugrxha force-pushed the handling-env-file-not-found branch 2 times, most recently from 3f6ba1c to b42914a Compare November 7, 2023 07:14
@ardinugrxha ardinugrxha marked this pull request as draft November 7, 2023 08:45
@ardinugrxha ardinugrxha marked this pull request as ready for review November 7, 2023 09:49
src/node_dotenv.cc Show resolved Hide resolved
src/node.cc Outdated Show resolved Hide resolved
src/node.cc Outdated Show resolved Hide resolved
@ardinugrxha ardinugrxha force-pushed the handling-env-file-not-found branch 2 times, most recently from 226b7f9 to f2365e5 Compare November 8, 2023 04:26
@anonrig anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 10, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 10, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@ardinugrxha
Copy link
Contributor Author

Seems several CI have failed even already retrying, is this expected? 🤔

@ardinugrxha
Copy link
Contributor Author

Seems several CI have failed even already retrying, is this expected? 🤔

Unit testing goes error on Windows, will check later and maybe come back with a new push

@debadree25 debadree25 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 13, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 13, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Trott Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 14, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 14, 2023
@nodejs-github-bot nodejs-github-bot merged commit 996d198 into nodejs:main Nov 14, 2023
46 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 996d198

targos pushed a commit that referenced this pull request Nov 23, 2023
PR-URL: #50588
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
martenrichter pushed a commit to martenrichter/node that referenced this pull request Nov 26, 2023
PR-URL: nodejs#50588
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Nov 28, 2023
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
PR-URL: #50588
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Dec 12, 2023
UlisesGascon pushed a commit that referenced this pull request Dec 19, 2023
PR-URL: #50588
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@jmjf
Copy link

jmjf commented Jan 11, 2024

@ardinugrxha

Is it intended that this change will break existing npm scripts and the CI processes that rely on them?

I have a test script that runs tests with --env-file=.env. The .env file is not checked in to my repo because it contains secrets.

This script worked in CI before 20.11 because CI has env variables set up in the CI environment. Now it fails.

My workaround will be to duplicate the test script without --env-file, but I'm not sure non-major releases in LTS should require workarounds.

@mbrevda mbrevda mentioned this pull request Jan 12, 2024
8 tasks
@acidoxee
Copy link

Hi, I have the same issue as @jmjf. Some of my scripts used --env-file .env as a non-mandatory env injection mechanism, but now after upgrading to Node 20.11.0, all of those scripts break if the file isn't there (which happens mostly in CI). There are workarounds, so I'll get by, but it's quite surprising seeing such a breaking change land in a minor version without warning. This change isn't even mentioned as a notable change in the changelog, it's only listed in the commits.

#50993 seems like a nice alternative that would have been nice to ship at the same time, but it's not there yet. Also, it may have been sweeter to ship it the other way around, like adding a --strict-env-file or something (that breaks if the env file isn't there) instead of changing the current way of working of the --env-file flag in a breaking way.

@anonrig
Copy link
Member

anonrig commented Jan 13, 2024

@acidoxee This is an experimental feature. Breaking changes are meant to happen.

@jmjf
Copy link

jmjf commented Jan 13, 2024 via email

@anonrig
Copy link
Member

anonrig commented Jan 13, 2024

--env-file=config has a stability index of 1.1 - Active Development. Let's take a look into the stability index (ref: https://nodejs.org/docs/latest/api/documentation.html#stability-index)

Stability: 1 - Experimental. The feature is not subject to [semantic versioning](https://semver.org/) rules. Non-backward compatible changes or removal may occur in any future release. Use of the feature is not recommended in production environments.
Experimental features are subdivided into stages:

1.0 - Early development. Experimental features at this stage are unfinished and subject to substantial change.
1.1 - Active development. Experimental features at this stage are nearing minimum viability.
1.2 - Release candidate. Experimental features at this stage are hopefully ready to become stable. No further breaking changes are anticipated but may still occur in response to user feedback. We encourage user testing and feedback so that we can know that this feature is ready to be marked as stable.

@acidoxee
Copy link

@acidoxee This is an experimental feature. Breaking changes are meant to happen.

Oh, my apologies then, I was pretty sure it was considered stable already for some reason. A notable change in the changelog would still have been nice, but you're 100% right to do breaking changes if it's experimental and if that leads to a better, thoughtful API in the end 👍

@BoscoDomingo
Copy link
Contributor

BoscoDomingo commented May 13, 2024

Hi, sorry for the noise this may cause:

Are you considering adding the optionality back? It'd be nice to see a --optional-env-file. Just now I was switching from dotenv-cli to the native loader, and having to create a duplicate script without the --env-file is not the best experience.

I can also try to do it myself, but I've 0 familiarity with Node's codebase and C++, when it might be an easy task for anyone more knowledgeable than me.

Edit: There's already a Feature Request for this, #50993, and a related issue #51451

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++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants