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: sync NODE_REPL_EXTERNAL_MODULE and kDisableNodeOptionsEnv #52905
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
fce6acc
to
c031cfb
Compare
IMO if it's a breaking change it should be semver-minor or notable-change (But as a triage member, this isn't my decision, nor my place) |
Failed to start CI⚠ No approving reviews found ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/9008046172 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No opinion from me on semver for this, but the code change LGTM.
changes: | ||
- version: | ||
- REPLACEME | ||
pr-url: https://github.com/nodejs/node/pull/52905 | ||
description: | ||
Remove the possibility to use this env var with | ||
kDisableNodeOptionsEnv for embedders. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Actually, with a little more thought, I think this is semver patch, so I agree with @RafaelGSS. But I'll defer to Releasers who think otherwise or collaborators more knowledgable about how these features are used. |
Whatever you say I'm happy with, I'm not a collaborator, so I really don't have much of an opinion (and experience) to really weigh in |
ccfa477
to
0b16434
Compare
I'm getting
I don't see why my changes would cause this. Any idea? @nodejs/build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a suggestion to add a comment in case the test needs to test NODE_OPTIONS in the future..
{ | ||
node::ProcessInitializationFlags::kNoInitializeV8, | ||
node::ProcessInitializationFlags::kNoInitializeNodeV8Platform, | ||
node::ProcessInitializationFlags::kDisableNodeOptionsEnv, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node::ProcessInitializationFlags::kDisableNodeOptionsEnv, | |
// This is used to test NODE_REPL_EXTERNAL_MODULE is disabled with | |
// kDisableNodeOptionsEnv. If other tests need NODE_OPTIONS support in the | |
// future, split this configuration out as a command line option. | |
node::ProcessInitializationFlags::kDisableNodeOptionsEnv, |
Fixes: #51227
Currently, I'm throwing kBootstrapError, but I'm almost sure this is not the appropriate error. In this case, should I create a new error?
Technically, this is a fix for
kDisableNodeOptionsEnv
, but it can be considered a breaking change for the ones relying on the current behavior (which is unlikely).