Allow node patch versions to be higher on runtime #1189

hashworks · 2022-01-28T12:11:18Z

This adjusts the runtime node version check to only compare major and minor version requirements. Systems that don't use the bundled NodeJS might update their node due to security patches, which would currently break OpenSearch Dashboards unnecessarily.

% grep 14.18 ../../package.json
    "node": "14.18.2",
% nvm use 12
Now using node v12.22.8 (npm v6.14.15)
% node node_version_validator.js
OpenSearch Dashboards does not support the current Node.js version v12.22.8. Please use Node.js ~v14.18.
% nvm use 14
Now using node v14.18.3 (npm v6.14.15)
% node node_version_validator.js
%

src/setup_node_env/node_version_validator.test.js

tmarkley · 2022-02-08T00:13:23Z

Linting failed. Run yarn lint and then yarn lint:es --fix.

tmarkley

Thanks for the contribution! This looks good to me.

boktorbb

Looks good. Thank you!

kavilla · 2022-02-09T07:23:51Z

Hello @hashworks, thanks for the changes! Also, seems to resolve a big headache from the community.

Verified it seems to work great!

I could be overthinking. But I think it might be worth to have some indication of the version of Node.js that OpenSearch Dashboards was built with in the error message. As a developer on this project, it is pretty easy to find which explicit version to use but it might not be for others. The reason why I suggest this is that there is an inherit risk enabling a version that was not used in development and that was verified in a release process, albeit it small since it is a patch version. I'd rather if we could guide people on the verified version but let them know that they are empowered to make the decision that they accept the risk of using any other patch version could save a potential deep dive.

That said, the before behavior was always used the bundled node however now the logic seems seems to increase in more support that would be helpful to begin add to the documentation site, or least in a markdown file.

kavilla · 2022-02-15T05:46:53Z

@hashworks any comment on the above? Let me know if I can answer any questions.

Thank you.

hashworks · 2022-02-17T14:39:42Z

Sorry for the delay, I've been a bit busy lately.

Well, (pkg && pkg.engines && pkg.engines.node) already provides the version it was packaged with (since this refers to the version in the package.json, which is a hard build requirement). I guess we could adjust the text:

OpenSearch Dashboards was build with v14.17.3 and does not support the current Node.js version v14.16.2. Please use Node.js ~v14.17.

kavilla · 2022-02-18T09:14:37Z

OpenSearch Dashboards was build with v14.17.3 and does not support the current Node.js version v14.16.2. Please use Node.js ~v14.17.

I think it would be built but I have definitely been wrong before. But I really like the message its helpful but provides a lot of insight without having to dive no further.

I +1 that message, @tmarkley, @boktorbb-amzn, @kgcreative, @keithhc2 any thoughts?

keithhc2 · 2022-02-18T18:35:46Z

Agree that it should be built. +1 from me on the message as well.

kavilla

LGTM! Thanks.

I see the 1.3 label is added to this, I'm hesitant to classify this as a minor version change since it's a major change in behavior (albeit it does a less strict check).

tmarkley · 2022-02-23T18:31:16Z

I see the 1.3 label is added to this, I'm hesitant to classify this as a minor version change since it's a major change in behavior (albeit it does a less strict check).

@kavilla that's reasonable, especially since 1.x is still on Node.js v10.

tmarkley

Thanks for the improvements! I think there's an opportunity to add a few more tests, but otherwise this looks great.

src/setup_node_env/node_version_validator.test.js

tmarkley · 2022-02-23T18:39:49Z

src/setup_node_env/node_version_validator.js

@@ -37,15 +37,18 @@ var pkg = require('../../package.json');
 var currentVersion = (process && process.version) || null;
 var rawRequiredVersion = (pkg && pkg.engines && pkg.engines.node) || null;
 var requiredVersion = rawRequiredVersion ? 'v' + rawRequiredVersion : rawRequiredVersion;
-var isVersionValid = !!currentVersion && !!requiredVersion && currentVersion === requiredVersion;
+var requiredVersionMajorMinor = requiredVersion.match(/^v(\d+\.\d+)/)[1];
+var isVersionValid = requiredVersionMajorMinor === currentVersion.match(/^v(\d+\.\d+)/)[1];

 // Validates current the NodeJS version compatibility when OpenSearch Dashboards starts.
 if (!isVersionValid) {
  var errorMessage =


Can we add at least one test to confirm that this error message spits out what we expect? I don't see any current tests that validate these regex expressions.

Added in 14166cd... but please verify that this is what you had in mind. I can't really test a fixed string because it changes with the used node version.

src/setup_node_env/node_version_validator.js

kavilla · 2022-04-07T12:36:43Z

I have moved this 2.1.0 because I think we should act on Tommy's responses but I think we can build this in a way that enables us to call this a feature. Sorry about the delay on this.

kavilla

Blocking to ensure 2.0.0-rc1 stability.

hashworks · 2022-04-14T15:24:10Z

I have moved this 2.1.0 because I think we should act on Tommy's responses but I think we can build this in a way that enables us to call this a feature. Sorry about the delay on this.

My bad, I didn't have time to fix things up.

This adjusts the runtime node version check to only compare major and minor version requirements. Systems that don't use the bundled NodeJS might update their node due to security patches, which would currently break OpenSearch Dashboards unecessarily. ```shell % grep 14.18 ../../package.json "node": "14.18.2", % nvm use 12 Now using node v12.22.8 (npm v6.14.15) % node node_version_validator.js OpenSearch Dashboards does not support the current Node.js version v12.22.8. Please use Node.js ~v14.18. % nvm use 14 Now using node v14.18.3 (npm v6.14.15) % node node_version_validator.js % ``` Signed-off-by: Justin Kromlinger <[email protected]>

Signed-off-by: Justin Kromlinger <[email protected]>

Tests versions that are greater than and less than the current version. Signed-off-by: Justin Kromlinger <[email protected]>

Signed-off-by: Justin Kromlinger <[email protected]>

codecov-commenter · 2022-04-28T18:16:20Z

Codecov Report

Merging #1189 (0218576) into main (1ad1a8b) will increase coverage by 0.00%.
The diff coverage is 75.00%.

@@           Coverage Diff           @@
##             main    #1189   +/-   ##
=======================================
  Coverage   68.09%   68.09%           
=======================================
  Files        3072     3072           
  Lines       59032    59034    +2     
  Branches     8928     8928           
=======================================
+ Hits        40198    40200    +2     
  Misses      16647    16647           
  Partials     2187     2187

Impacted Files	Coverage Δ
src/setup_node_env/node_version_validator.js	`72.72% <75.00%> (+6.06%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ad1a8b...0218576. Read the comment docs.

ashwin-pc

Thanks for adding these tests. Love how you are testing for the different major, minor and patch versions.

tmarkley

Thanks so much for the improvements! This looks great.

boktorbb

Great work! Thank you

kavilla · 2022-06-01T23:38:24Z

Just wanted to follow up, as you stated before it help people who do not have the pre-bundled Node. Will just re-hash that behavior might seem inconsistent for end-users and developers who do not have the insight here. Perhaps we can add the needs-documentation label to create an issue for this behavior. Or do we think maybe we can pivot this to use: #1219, where we just check if this environment variable is set and just skip the verification logic in general, or keep this original logic intact and only execute this new logic if the environment variable is set.

Good as is, potentially needs docs or env variable

ashwin-pc · 2022-06-02T20:44:38Z

@kavilla Added the label and we can address the node_home env when we get to #1219. There we can add a check to skip this if the flag is set. since we dont have support for that flag yet, I dont think it makes sense in introducing that logic right now.

This adjusts the runtime node version check to only compare major and minor version requirements. Systems that don't use the bundled NodeJS might update their node due to security patches, which would currently break OpenSearch Dashboards unnecessarily. ```shell % grep 14.18 ../../package.json "node": "14.18.2", % nvm use 12 Now using node v12.22.8 (npm v6.14.15) % node node_version_validator.js OpenSearch Dashboards does not support the current Node.js version v12.22.8. Please use Node.js ~v14.18. % nvm use 14 Now using node v14.18.3 (npm v6.14.15) % node node_version_validator.js % ``` * Mention Node.js version Dashboards was built with in runtime check error * Use template literals in node version validator * Add additional node validator tests, cleanup * Don't allow node patch versions to be lower on runtime Signed-off-by: Justin Kromlinger <[email protected]> (cherry picked from commit 0a24652)

…#1189) This adjusts the runtime node version check to only compare major and minor version requirements. Systems that don't use the bundled NodeJS might update their node due to security patches, which would currently break OpenSearch Dashboards unnecessarily. ```shell % grep 14.18 ../../package.json "node": "14.18.2", % nvm use 12 Now using node v12.22.8 (npm v6.14.15) % node node_version_validator.js OpenSearch Dashboards does not support the current Node.js version v12.22.8. Please use Node.js ~v14.18. % nvm use 14 Now using node v14.18.3 (npm v6.14.15) % node node_version_validator.js % ``` * Mention Node.js version Dashboards was built with in runtime check error * Use template literals in node version validator * Add additional node validator tests, cleanup * Don't allow node patch versions to be lower on runtime Signed-off-by: Justin Kromlinger <[email protected]>

This adjusts the runtime node version check to only compare major and minor version requirements. Systems that don't use the bundled NodeJS might update their node due to security patches, which would currently break OpenSearch Dashboards unnecessarily. ```shell % grep 14.18 ../../package.json "node": "14.18.2", % nvm use 12 Now using node v12.22.8 (npm v6.14.15) % node node_version_validator.js OpenSearch Dashboards does not support the current Node.js version v12.22.8. Please use Node.js ~v14.18. % nvm use 14 Now using node v14.18.3 (npm v6.14.15) % node node_version_validator.js % ``` * Mention Node.js version Dashboards was built with in runtime check error * Use template literals in node version validator * Add additional node validator tests, cleanup * Don't allow node patch versions to be lower on runtime Signed-off-by: Justin Kromlinger <[email protected]> (cherry picked from commit 0a24652)

…#1189) This adjusts the runtime node version check to only compare major and minor version requirements. Systems that don't use the bundled NodeJS might update their node due to security patches, which would currently break OpenSearch Dashboards unnecessarily. ```shell % grep 14.18 ../../package.json "node": "14.18.2", % nvm use 12 Now using node v12.22.8 (npm v6.14.15) % node node_version_validator.js OpenSearch Dashboards does not support the current Node.js version v12.22.8. Please use Node.js ~v14.18. % nvm use 14 Now using node v14.18.3 (npm v6.14.15) % node node_version_validator.js % ``` * Mention Node.js version Dashboards was built with in runtime check error * Use template literals in node version validator * Add additional node validator tests, cleanup * Don't allow node patch versions to be lower on runtime Signed-off-by: Justin Kromlinger <[email protected]>

…#1189) (opensearch-project#1677) This adjusts the runtime node version check to only compare major and minor version requirements. Systems that don't use the bundled NodeJS might update their node due to security patches, which would currently break OpenSearch Dashboards unnecessarily. ```shell % grep 14.18 ../../package.json "node": "14.18.2", % nvm use 12 Now using node v12.22.8 (npm v6.14.15) % node node_version_validator.js OpenSearch Dashboards does not support the current Node.js version v12.22.8. Please use Node.js ~v14.18. % nvm use 14 Now using node v14.18.3 (npm v6.14.15) % node node_version_validator.js % ``` * Mention Node.js version Dashboards was built with in runtime check error * Use template literals in node version validator * Add additional node validator tests, cleanup * Don't allow node patch versions to be lower on runtime Signed-off-by: Justin Kromlinger <[email protected]> (cherry picked from commit 0a24652)

…#1189) This adjusts the runtime node version check to only compare major and minor version requirements. Systems that don't use the bundled NodeJS might update their node due to security patches, which would currently break OpenSearch Dashboards unnecessarily. ```shell % grep 14.18 ../../package.json "node": "14.18.2", % nvm use 12 Now using node v12.22.8 (npm v6.14.15) % node node_version_validator.js OpenSearch Dashboards does not support the current Node.js version v12.22.8. Please use Node.js ~v14.18. % nvm use 14 Now using node v14.18.3 (npm v6.14.15) % node node_version_validator.js % ``` * Mention Node.js version Dashboards was built with in runtime check error * Use template literals in node version validator * Add additional node validator tests, cleanup * Don't allow node patch versions to be lower on runtime Signed-off-by: Justin Kromlinger <[email protected]>

hashworks requested a review from a team as a code owner January 28, 2022 12:11

tmarkley suggested changes Feb 3, 2022

View reviewed changes

src/setup_node_env/node_version_validator.test.js Outdated Show resolved Hide resolved

hashworks force-pushed the feature/majorMinorRuntimeCheck branch from b40dbb3 to 03f47ec Compare February 4, 2022 03:49

hashworks requested a review from tmarkley February 4, 2022 03:51

hashworks force-pushed the feature/majorMinorRuntimeCheck branch from 03f47ec to d3a773d Compare February 8, 2022 07:56

jcgraybill mentioned this pull request Feb 8, 2022

[FEATURE] Add support for OPENSEARCH_DASHBOARDS_NODE_HOME (or OSD_NODE_HOME) #1219

Closed

tmarkley previously approved these changes Feb 8, 2022

View reviewed changes

boktorbb self-requested a review February 8, 2022 21:03

boktorbb previously approved these changes Feb 8, 2022

View reviewed changes

tmarkley added the v1.3.0 label Feb 8, 2022

hashworks dismissed stale reviews from boktorbb and tmarkley via e34f2db February 19, 2022 13:17

hashworks force-pushed the feature/majorMinorRuntimeCheck branch from e34f2db to 3e94742 Compare February 21, 2022 19:32

kavilla approved these changes Feb 23, 2022

View reviewed changes

tmarkley reviewed Feb 23, 2022

View reviewed changes

kavilla removed the v1.3.0 label Feb 23, 2022

tmarkley added the v2.0.0 label Mar 3, 2022

kavilla added v2.1.0 and removed v2.0.0 labels Apr 7, 2022

kavilla previously requested changes Apr 14, 2022

View reviewed changes

hashworks force-pushed the feature/majorMinorRuntimeCheck branch 2 times, most recently from d2b7bd4 to fa69e60 Compare April 14, 2022 14:45

hashworks added 6 commits April 28, 2022 16:51

Mention Node.js version Dashboards was built with in runtime check error

56fbf8f

Signed-off-by: Justin Kromlinger <[email protected]>

Use template literals in node version validator

9d45760

Signed-off-by: Justin Kromlinger <[email protected]>

Add additional node validator tests, cleanup

644d3ba

Tests versions that are greater than and less than the current version. Signed-off-by: Justin Kromlinger <[email protected]>

Test error message of node version validator

14166cd

Signed-off-by: Justin Kromlinger <[email protected]>

Don't allow node patch versions to be lower on runtime

0218576

Signed-off-by: Justin Kromlinger <[email protected]>

hashworks force-pushed the feature/majorMinorRuntimeCheck branch from a28c8ed to 0218576 Compare April 28, 2022 15:08

hashworks requested review from kavilla and tmarkley April 28, 2022 15:10

hashworks changed the title ~~Allow node patch versions to be different on runtime~~ Allow node patch versions to be higher on runtime Apr 28, 2022

ashwin-pc approved these changes Apr 29, 2022

View reviewed changes

tmarkley approved these changes May 2, 2022

View reviewed changes

tmarkley added the backport 2.x label May 2, 2022

boktorbb approved these changes May 2, 2022

View reviewed changes

ashwin-pc added the needs-documentation label Jun 2, 2022

tmarkley merged commit 0a24652 into opensearch-project:main Jun 6, 2022

opensearch-trigger-bot bot mentioned this pull request Jun 6, 2022

[Backport 2.x] Allow node patch versions to be higher on runtime #1677

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow node patch versions to be higher on runtime #1189

Allow node patch versions to be higher on runtime #1189

hashworks commented Jan 28, 2022

tmarkley commented Feb 8, 2022

tmarkley left a comment

boktorbb left a comment

kavilla commented Feb 9, 2022 •

edited

Loading

kavilla commented Feb 15, 2022

hashworks commented Feb 17, 2022

kavilla commented Feb 18, 2022

keithhc2 commented Feb 18, 2022

kavilla left a comment

tmarkley commented Feb 23, 2022

tmarkley left a comment

tmarkley Feb 23, 2022

hashworks Apr 14, 2022 •

edited

Loading

kavilla commented Apr 7, 2022

kavilla left a comment

hashworks commented Apr 14, 2022

codecov-commenter commented Apr 28, 2022

ashwin-pc left a comment

tmarkley left a comment

boktorbb left a comment

kavilla commented Jun 1, 2022

ashwin-pc commented Jun 2, 2022

Allow node patch versions to be higher on runtime #1189

Allow node patch versions to be higher on runtime #1189

Conversation

hashworks commented Jan 28, 2022

tmarkley commented Feb 8, 2022

tmarkley left a comment

Choose a reason for hiding this comment

boktorbb left a comment

Choose a reason for hiding this comment

kavilla commented Feb 9, 2022 • edited Loading

kavilla commented Feb 15, 2022

hashworks commented Feb 17, 2022

kavilla commented Feb 18, 2022

keithhc2 commented Feb 18, 2022

kavilla left a comment

Choose a reason for hiding this comment

tmarkley commented Feb 23, 2022

tmarkley left a comment

Choose a reason for hiding this comment

tmarkley Feb 23, 2022

Choose a reason for hiding this comment

hashworks Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

kavilla commented Apr 7, 2022

kavilla left a comment

Choose a reason for hiding this comment

hashworks commented Apr 14, 2022

codecov-commenter commented Apr 28, 2022

Codecov Report

ashwin-pc left a comment

Choose a reason for hiding this comment

tmarkley left a comment

Choose a reason for hiding this comment

boktorbb left a comment

Choose a reason for hiding this comment

kavilla commented Jun 1, 2022

ashwin-pc commented Jun 2, 2022

kavilla commented Feb 9, 2022 •

edited

Loading

hashworks Apr 14, 2022 •

edited

Loading