introduce NODEJS_ORG_MIRROR and IOJS_ORG_MIRROR #878
Conversation
|
👍 |
|
I can't judge the build & tests but source changes look good. 👍 |
|
LGTM |
|
Should we do a |
Like a deprecated warning? +1 |
|
@rvagg it's primarily that it's an env var that while I don't intend to change, I might - a warning seems reasonable so people know to switch. |
rvagg
force-pushed
the
nodejs_org_mirror
branch
from
6a87f83 to
3350bf9
Compare
|
Added a warning, run the tests to see it output: @bnoordhuis can I ask for a review here as well please? @zkat @iarna are there any guidelines on outputting warnings with |
|
|
||
| var release = processRelease([], { opts: {} }, 'v3.2.24', { | ||
| name: 'io.js', | ||
| headersUrl: 'http://foo.bar/v3.2.24/iojs-v3.2.24-headers.tar.gz' |
There was a problem hiding this comment.
You use 'https://nodejs.org/` in the first two tests but http://foo.bar here and in the next test. Is that intentional?
There was a problem hiding this comment.
No, search/replace error, the test should find that process.release is mostly ignored and it's not testing that as it is now. Thanks for noticing, will fix!
|
JS changes LGTM with a question, no opinion on the docker shell script. There are some long lines but that's not really a thing with node-gyp. |
Deprecate NVM_NODEJS_ORG_MIRROR and NVM_IOJS_ORG_MIRROR as they were only ever intended to be internal nvm environment variables. These will be removed in the next semver-major release.
rvagg
force-pushed
the
nodejs_org_mirror
branch
from
3350bf9 to
95ed796
Compare
Deprecate NVM_NODEJS_ORG_MIRROR and NVM_IOJS_ORG_MIRROR as they were only ever intended to be internal nvm environment variables. These will be removed in the next semver-major release. PR-URL: #878 Reviewed-By: ralphtheninja Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
/R= @nodejs/build @ljharb
to be followed soon by a semver-major that removes the
NVM_variants as these cause grief, see #787