Limit Node.js version resolved when a wide range is used #1498
Conversation
Updates the Node.js version resolver with the following features: - detect when a wide version range is in use - limit the resolved version to the latest LTS unless a higher version is explicitly requested - output the resolved data as JSON so that it's easier to understand
This is a sanity check since rebuilding the binary after making changes is a manual process. This will ensure that the binary can't be updated if there are failing tests.
The install process is updated to: - read the new JSON output from the Node.js version resolver binary - emit a warning if a wide version range is used - emit a warning if the LTS version limit had to be enforced
Signed-off-by: Colin Casey <[email protected]>
schneems
left a comment
schneems
left a comment
There was a problem hiding this comment.
Made some suggestions, see comments.
| if [[ "$uses_wide_range" == "true" ]]; then | ||
| echo "! The requested Node.js version is using a wide range ($requested_version) that can resolve to a major version" | ||
| echo " you may not expect. Limiting the requested range to a major range like \`24.x\` is recommended." | ||
| echo " https://devcenter.heroku.com/articles/nodejs-support#specifying-a-node-js-version" |
There was a problem hiding this comment.
Devcenter suggestion: The link says what to do:
Because Node does regular security releases on all supported major versions, we recommend specifying a major range to get security updates automatically, for example, 22.x.
But it never says what not to do. I reccomend we add a counter recommendation something like "We recommend against using wide version ranges such as >= 22 and instead recommend sticking to a version that cannot resolve to a higher major version such as 22.x
|
|
||
| #[test] | ||
| fn resolve_version_when_wide_range_is_used_and_explicitly_requesting_range_beyond_lts() { | ||
| let wide_requirement = Requirement::from_str(">=25.x").unwrap(); |
There was a problem hiding this comment.
Optional suggestion: When you change the constant to make the LTS version 24, all of these will start failing.
My suggestion would be:
- Inject the maximum version into the function instead of using a constant, possibly using a bon builder to make it the default. Then the tests can stay static even if the default changes or
- Generate the number "25" based on the constant (or hardcode a new constant) and use
format!in these tests.
|
|
||
| if [[ "$uses_wide_range" == "true" ]]; then | ||
| echo "! The requested Node.js version is using a wide range ($requested_version) that can resolve to a major version" | ||
| echo " you may not expect. Limiting the requested range to a major range like \`24.x\` is recommended." |
There was a problem hiding this comment.
Optional suggestion: Instead of hard-coding 24.x you can return that value from rust so when you update the value in your main.rs it's updated here as well.
2 participants
Updates the Node.js version resolver with the following features:
This allows the buildpack to output:
W-20075501