New: Adding NodeJS version check to Release script #834
Conversation
|
Verified that @JustinHoldstock has signed the CLA. Thanks for the pull request! |
build/release.sh
Outdated
| @@ -236,6 +253,8 @@ while getopts "mnp" opt; do | |||
| esac | |||
| done | |||
|
|
|||
| # Before running release, make sure using correct version of NodeJS | |||
| validate_node_version; | |||
There was a problem hiding this comment.
this might make more sense added to the push_new_release function like we do with other checks?
build/validateNodeVersion.js
Outdated
| } | ||
| } | ||
|
|
||
| https.get(NODE_JS_VERSION_LIST_URL, response => { |
There was a problem hiding this comment.
No harm in bringing in axios as a dev dependency to accomplish this (IMO).
There was a problem hiding this comment.
Done! Really good idea, thank you
build/validateNodeVersion.js
Outdated
| body += data; | ||
| }); | ||
|
|
||
| response.on('end', validateNodeVersion); |
There was a problem hiding this comment.
Where does this data get used? ValidateNodeVersion doesn't take in any parameters
There was a problem hiding this comment.
See use of body.
| const SUPPORTED_LTS = "Carbon"; | ||
| const VERSION = process.versions.node; | ||
| const NODE_JS_VERSION_LIST_URL="https://nodejs.org/dist/index.json" | ||
| const MIN_MAJOR_SUPPORT = 8; |
There was a problem hiding this comment.
Are we still relying solely off the information from nodejs.org? In that case why are these constants needed?
There was a problem hiding this comment.
We still want to enforce our minimum requirement before doing a compare against the NodeJS listings. I'm actually going to do that check first, so we don't have to make the network request UNLESS we've determined it's >= than our required version
build/validateNodeVersion.js
Outdated
|
|
||
| function fail() { | ||
| body = ''; | ||
| console.log(false); |
There was a problem hiding this comment.
Do we need to throw an error at some point to get the release script to break?
There was a problem hiding this comment.
console.log() is picked up by the bash script that invokes this. Do you think we also need to throw an error if the parent bash script is halting all execution after receiving a false signal from this?
There was a problem hiding this comment.
can we do this with just return values and not console.log()?
| */ | ||
| function compareVersion(major, minor, patch) { | ||
| // If major version is greater, it's valid | ||
| if (major > MIN_MAJOR_SUPPORT) { |
There was a problem hiding this comment.
Can this be a problem, lets say someone is using node 10? There could be breaking changes
There was a problem hiding this comment.
compareVersion() only compares against our minimum requirements of node 8.9.4. After this is executed, we do a check against LTS versions of node, which are supported (at least) by conventional-github-releaser.
…ew into Enable-LTS-check-for-release-script
…inHoldstock/box-content-preview into Enable-LTS-check-for-release-script
build/validateNodeVersion.js
Outdated
| const MIN_MINOR_SUPPORT = 9; | ||
| const MIN_PATCH_SUPPORT = 4; | ||
|
|
||
| function fail() { |
There was a problem hiding this comment.
Maybe drop a doc string on these explaining what the process.exit does?
|
|
||
|
|
||
| /** | ||
| * Exit with code 1 so that calling script can see that current node version was NOT valid. |
No description provided.