Skip to content

src: use for loop in Dotenv::GetPathFromArgs - #54385

Closed
avivkeller wants to merge 2 commits into
nodejs:mainfrom
avivkeller:patch-93
Closed

src: use for loop in Dotenv::GetPathFromArgs#54385
avivkeller wants to merge 2 commits into
nodejs:mainfrom
avivkeller:patch-93

Conversation

@avivkeller

@avivkelleravivkeller commented Aug 14, 2024

Copy link
Copy Markdown
Member

This PR replaces the old while loop with a newer for loop.

Fixes the following edge cases:

node script.js --env-file .env
node --env-file-ABCD .env
node -- --env-file .env
node -invalid --env-file .env # this will error, but the env file is still processed

Unfortunately, this introduces an edge case where some argument parameters, such as --eval <...> cause any future --env-file arguments to be ignored.

node --eval "1+1" --env-file notparsed.env

Fixes#54255
Fixes#54232
Related to #54237

@nodejs-github-botnodejs-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 Aug 14, 2024
@avivkelleravivkeller added dotenv Issues and PRs related to .env file parsing commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Aug 14, 2024

@anonriganonrig left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you add a cc test for testing only this function and every edge case you're targeting?

Comment threadsrc/node_dotenv.cc Outdated
Comment threadtest/parallel/test-dotenv-edge-cases.js Outdated
@avivkeller

avivkeller commented Aug 15, 2024

Copy link
Copy Markdown
MemberAuthor

@anonrig would you mind restarting the Github CI now that #54391 landed?

Actually I'm going to rebase for it.

Comment threadsrc/node_dotenv.cc Outdated
@codecov

codecovBot commented Aug 15, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 87.07%. Comparing base (2c14615) to head (35c4dae).
Report is 268 commits behind head on main.

Files with missing linesPatch %Lines
src/node_dotenv.cc80.00%0 Missing and 2 partials ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #54385 +/- ##
==========================================
- Coverage 87.08% 87.07% -0.01% 
==========================================
Files 648 648 Lines 182341 182338 -3 Branches 34982 34981 -1 ==========================================
- Hits 158783 158773 -10 - Misses 16831 16833 +2 - Partials 6727 6732 +5 
Files with missing linesCoverage Δ
src/node_dotenv.cc81.36% <80.00%> (-0.35%)⬇️

... and 24 files with indirect coverage changes

Comment threadsrc/node_dotenv.cc
@avivkeller

Copy link
Copy Markdown
MemberAuthor

@anonrig Do you think this should land (eventually), or it should be held-off, and a redone GetPathFromArgs would be better, one that doesn't have these edge cases?

@avivkeller

Copy link
Copy Markdown
MemberAuthor

Closing in favor of #54913

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.dotenvIssues and PRs related to .env file parsingneeds-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dotenv::GetPathFromArgs matches --env-file* Script argument --env-file is wrongly used as node option

3 participants

@avivkeller@anonrig@nodejs-github-bot