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.

Sign up for GitHub

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

Jump to bottom

src: remove unused persistent properties from env #15096

Closed
wants to merge 1 commit into from
Closed

src: remove unused persistent properties from env #15096

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

Removes one unused and two write-only properties as general cleanup.

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

src/env

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. tls Issues and PRs related to the tls subsystem. labels Aug 30, 2017
@mscdex
Copy link
Contributor

mscdex commented Aug 30, 2017

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

@mscdex
Copy link
Contributor

mscdex commented Aug 30, 2017

LGTM

@danbev
Copy link
Contributor

danbev commented Aug 30, 2017

Not related to this PR, but the failing ci job seems to be related to an incorrect value for Make's -j command:

+ NODE_TEST_DIR=/home/iojs/node-tmp PYTHON=python FLAKY_TESTS=dontcare TEST_CI_ARGS= make run-ci -j 0
make: the '-j' option requires a positive integer argument
Usage: make [options] [target] ...

I'm not sure if I have the permission to change this (and don't know where either) so wanted to bring it up.

@jasnell
Copy link
Member

jasnell commented Aug 30, 2017

The issue with CI has been reported to @nodejs/build

@gibfahn
Copy link
Member

gibfahn commented Aug 31, 2017

@danbev CI issue is nodejs/build#857 (should be fixed, comment there if it isn't).

And for general info currently only @nodejs/jenkins-admins have access to change job configs, issue to change that is nodejs/build#838.

jasnell pushed a commit that referenced this pull request Sep 1, 2017
@addaleax @jasnell
src: remove unused persistent properties from env
f91897e 
PR-URL: #15096
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Brian White <[email protected]>
@jasnell
Copy link
Member

jasnell commented Sep 1, 2017

Landed in f91897e

@jasnell jasnell closed this Sep 1, 2017
addaleax added a commit to addaleax/ayo that referenced this pull request Sep 5, 2017
@addaleax
src: remove unused persistent properties from env
4c2115b 
PR-URL: nodejs/node#15096
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Brian White <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
@addaleax @MylesBorins
src: remove unused persistent properties from env
920a758 
PR-URL: #15096
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Brian White <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Sep 11, 2017
@addaleax @MylesBorins
src: remove unused persistent properties from env
46b33a3 
PR-URL: #15096
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Brian White <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
@addaleax @MylesBorins
src: remove unused persistent properties from env
a83d427 
PR-URL: #15096
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Brian White <[email protected]>
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@addaleax addaleax deleted the env-remove-unused-persistents branch September 20, 2017 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@cjihrig cjihrig cjihrig approved these changes

@benjamingr benjamingr benjamingr approved these changes

@jasnell jasnell jasnell approved these changes

@danbev danbev danbev approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@addaleax @mscdex @danbev @jasnell @gibfahn @MylesBorins @benjamingr @cjihrig @BridgeAR @nodejs-github-bot