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

watch: reload changes in contents of --env-file #54109

Merged
merged 2 commits into from
Aug 5, 2024

Conversation

marekpiechut
Copy link
Contributor

Make sure we watch and reload on env file changes.
Ignore env file in parent process, so child process can reload current vars when we recreate it.

Fixes: #54001

I've noticed now that it's probably a duplicate of #54033.

Feel free to close or reuse tests from here in the other PR if you plan on merging it.
It was a nice small bug to get feet wet with Node internals anyway. 👋

Make sure we watch and reload on env file changes.

Ignore env file in parent process, so child process can reload
current vars when we recreate it.

Fixes: nodejs#54001
@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 Jul 29, 2024
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 31, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 31, 2024
@nodejs-github-bot
Copy link
Collaborator

Copy link

codecov bot commented Jul 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.07%. Comparing base (cf9a814) to head (a8eecc8).
Report is 540 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #54109   +/-   ##
=======================================
  Coverage   87.07%   87.07%           
=======================================
  Files         643      643           
  Lines      181590   181576   -14     
  Branches    34893    34892    -1     
=======================================
- Hits       158114   158109    -5     
+ Misses      16754    16746    -8     
+ Partials     6722     6721    -1     
Files with missing lines Coverage Δ
src/node.cc 74.03% <100.00%> (ø)

... and 31 files with indirect coverage changes

@marekpiechut
Copy link
Contributor Author

Fixed linter issues.

@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 2, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 2, 2024
@nodejs-github-bot
Copy link
Collaborator

@marekpiechut
Copy link
Contributor Author

I see there are test failures in the latest check. I tried to look into them, but they all look weird. There are some unrelated test failures and build issues on Windows, etc.

@MoLow @anonrig could you give some advice how to continue with that?

@MoLow
Copy link
Member

MoLow commented Aug 4, 2024

none of those seem related

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@MoLow MoLow added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Aug 5, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 5, 2024
@nodejs-github-bot nodejs-github-bot merged commit a816688 into nodejs:main Aug 5, 2024
59 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in a816688

@marekpiechut marekpiechut deleted the watch-env-file branch August 5, 2024 11:25
@tniessen
Copy link
Member

tniessen commented Aug 5, 2024

Are there no NODE_OPTIONS that should affect the parent process?

@MoLow
Copy link
Member

MoLow commented Aug 5, 2024

@tniessen I am not sure what you mean? can you elaborate?
passing --env-file will be correctly forwarded to inner process regardless of if it is used with NODE_OPTIONS or without

targos pushed a commit that referenced this pull request Aug 14, 2024
Make sure we watch and reload on env file changes.

Ignore env file in parent process, so child process can reload
current vars when we recreate it.

Fixes: #54001
PR-URL: #54109
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Aug 19, 2024
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024
Sign up for free to 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-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--env-file watch
6 participants