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

process: generate list of allowed env flags programmatically by addaleax · Pull Request #22638 · nodejs/node · GitHub

/ node Public

process: generate list of allowed env flags programmatically - #22638

Closed
addaleax wants to merge 4 commits into
nodejs:masterfrom
addaleax:allowed-in-env-flags
ConversationCommits (4)ChecksFiles changed
Closed

process: generate list of allowed env flags programmatically#22638
addaleax wants to merge 4 commits into
nodejs:masterfrom
addaleax:allowed-in-env-flags

Conversation

addaleax commented Aug 31, 2018

Copy link
Copy Markdown
Member

Avoids having a separate, second source of truth on this matter.

The second diff is best viewed in whitespace-adjusted mode (append ?w=1 to GitHub URL).

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

nodejs-github-bot commented Aug 31, 2018

Copy link
Copy Markdown
Collaborator

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/757/pipeline

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 31, 2018
addaleax force-pushed the allowed-in-env-flags branch from 901d187 to 290af4b Compare August 31, 2018 22:54
BridgeAR reviewed Sep 2, 2018
View reviewed changes
Comment thread lib/internal/bootstrap/node.js Outdated

BridgeAR Sep 2, 2018 •
edited
Loading

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

I would prefer to pass through filter options into getOptions instead of doing the filtering here.
Since it has a string option at the moment, it would likely be best to accept an object for the options instead.

Another alternative to the current way would be to only iterate once over the entries (this is also nicer to read for me):

const allowedV8EnvironmentFlags = [];
const allowedNodeEnvironmentFlags = [];
for (const [name, info] of options) {
  if (info.envVarSettings === kAllowedInEnvironment) {
    if (info.type === KV8Option) {
      allowedV8EnvironmentFlags.push(name);
    } else {
      allowedNodeEnvironmentFlags.push(name);
    }
  }
}

addaleax Sep 2, 2018

Copy link
Copy Markdown
Member Author

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

I’ve gone with the second option for now, but yes, ultimately it might be better to do this in C++ land.

Comment thread lib/internal/bootstrap/node.js Outdated

BridgeAR Sep 2, 2018

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

Nit: retrieving the value multiple times is not necessary.

addaleax force-pushed the allowed-in-env-flags branch from 290af4b to fa245b8 Compare September 2, 2018 13:20

addaleax commented Sep 5, 2018

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/17027/

addaleax force-pushed the allowed-in-env-flags branch from fa245b8 to 1f37916 Compare September 5, 2018 10:51

addaleax commented Sep 5, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

New CI: https://ci.nodejs.org/job/node-test-pull-request/17028/ (:heavy_check_mark:)

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Sep 5, 2018

addaleax commented Sep 9, 2018

Copy link
Copy Markdown
Member Author

Landed in 995782c, d994e26

addaleax closed this Sep 9, 2018
addaleax deleted the allowed-in-env-flags branch September 9, 2018 14:23
addaleax added a commit that referenced this pull request Sep 9, 2018
process: generate list of allowed env flags programmatically
995782c 
Avoids having a separate, second source of truth on this matter.

PR-URL: #22638
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax added a commit that referenced this pull request Sep 9, 2018
lib: generate allowedNodeEnvironmentFlags lazily
d994e26 
PR-URL: #22638
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

targos commented Sep 10, 2018

Copy link
Copy Markdown
Member

This needs a backport PR to land on v10.x-staging

addaleax added a commit that referenced this pull request Sep 13, 2018
process: generate list of allowed env flags programmatically
b8321bf 
Avoids having a separate, second source of truth on this matter.

PR-URL: #22638
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax added a commit that referenced this pull request Sep 13, 2018
lib: generate allowedNodeEnvironmentFlags lazily
c7b8ad8 
PR-URL: #22638
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax mentioned this pull request Sep 13, 2018
Closed
targos pushed a commit that referenced this pull request Sep 15, 2018
process: generate list of allowed env flags programmatically
1e07002 
Avoids having a separate, second source of truth on this matter.

Backport-PR-URL: #22847
PR-URL: #22638
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Sep 15, 2018
lib: generate allowedNodeEnvironmentFlags lazily
a793163 
Backport-PR-URL: #22847
PR-URL: #22638
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos mentioned this pull request Sep 18, 2018
Merged
targos pushed a commit that referenced this pull request Sep 19, 2018
process: generate list of allowed env flags programmatically
cfced0c 
Avoids having a separate, second source of truth on this matter.

Backport-PR-URL: #22847
PR-URL: #22638
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Sep 19, 2018
lib: generate allowedNodeEnvironmentFlags lazily
60dc374 
Backport-PR-URL: #22847
PR-URL: #22638
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Sep 20, 2018
process: generate list of allowed env flags programmatically
4ab9d6f 
Avoids having a separate, second source of truth on this matter.

Backport-PR-URL: #22847
PR-URL: #22638
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Sep 20, 2018
lib: generate allowedNodeEnvironmentFlags lazily
2abdbf4 
Backport-PR-URL: #22847
PR-URL: #22638
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
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
BridgeAR BridgeAR approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL