FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

(v7.x backport) src: allow CLI args in env with NODE_OPTIONS by sam-github · Pull Request #12648 · nodejs/node · GitHub

/ node Public

(v7.x backport) src: allow CLI args in env with NODE_OPTIONS - #12648

Merged
sam-github merged 1 commit into
nodejs:v7.x-stagingfrom
sam-github:backport-12028-to-v7.x
May 16, 2017
ConversationCommits (1)ChecksFiles changed
Merged

(v7.x backport) src: allow CLI args in env with NODE_OPTIONS#12648
sam-github merged 1 commit into
nodejs:v7.x-stagingfrom
sam-github:backport-12028-to-v7.x

Conversation

sam-github commented Apr 25, 2017

Copy link
Copy Markdown
Contributor

See #12647, needed backport because of a trivial conflict against #10156 which touched the same long, long line in vcbuild.bat.

Not all CLI options are supported, those that are problematic from a
security or implementation point of view are disallowed, as are ones
that are inappropriate (for example, -e, -p, --i), or that only make
sense when changed with code changes (such as options that change the
javascript syntax or add new APIs).

PR-URL: #12028
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Michael Dawson michael_dawson@ca.ibm.com
Reviewed-By: Refael Ackermann refack@gmail.com
Reviewed-By: Gibson Fahnestock gibfahn@gmail.com
Reviewed-By: Bradley Farias bradley.meck@gmail.com

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. v7.x windows Issues and PRs related to the Windows platform. labels Apr 25, 2017
sam-github added lts-agenda semver-minor PRs that contain new features and should be released in the next minor version. labels Apr 25, 2017

sam-github commented Apr 25, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I request that this be landed on the next 7.x that is accepting minors. @nodejs/lts

I'm also going to request backporting to 6.x once its been in 7.x long enough to prove itself, but 6.x has non-trivial conflicts, I have to work through the actual C++ code and re-apply the changes. I'm doing that now.

evanlucas force-pushed the v7.x-staging branch 2 times, most recently from d4ceb59 to 69a8053 Compare May 3, 2017 12:56
sam-github mentioned this pull request May 9, 2017
Closed

gibfahn commented May 10, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Needs a rebase

sam-github force-pushed the backport-12028-to-v7.x branch from 708dabe to cdd7f62 Compare May 10, 2017 15:35

sam-github commented May 10, 2017

Copy link
Copy Markdown
Contributor Author

rebased without conflicts

jasnell approved these changes May 10, 2017
View reviewed changes

jasnell 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM if CI is green.

jasnell commented May 10, 2017

Copy link
Copy Markdown
Member

The v7.x branch is not an LTS branch so this does not need to be on the LTS WG agenda.

src: allow CLI args in env with NODE_OPTIONS
34e8eb6 
Not all CLI options are supported, those that are problematic from a
security or implementation point of view are disallowed, as are ones
that are inappropriate (for example, -e, -p, --i), or that only make
sense when changed with code changes (such as options that change the
javascript syntax or add new APIs).

PR-URL: nodejs#12028
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
sam-github force-pushed the backport-12028-to-v7.x branch from cdd7f62 to 34e8eb6 Compare May 16, 2017 19:43
sam-github merged commit 34e8eb6 into nodejs:v7.x-staging May 16, 2017
sam-github deleted the backport-12028-to-v7.x branch May 16, 2017 19:45

sam-github commented May 18, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@jasnell I don't know what I was thinking (I think I got confused between the 3 or 4 PRs I was doing CI runs on), but I landed this without CI, and its broken. Can I force-push it off of v7.x-staging? Its currently the head commit.

sam-github mentioned this pull request May 18, 2017
Merged
4 tasks

sam-github commented May 18, 2017

Copy link
Copy Markdown
Contributor Author

Got an OK from @evanlucas on IRC, and force-pushed this of of v7.x-staging. Trying again in #13063

aliscco mentioned this pull request Apr 27, 2023
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes

Assignees

No one assigned

Labels

build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. semver-minor PRs that contain new features and should be released in the next minor version. windows Issues and PRs related to the Windows platform.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL