Doesn't start with nodejs 14.20.0

[sipopo]

Description

Resolve bug the application doesn't start with nodejs version 14.20.0

Issues Resolved

[BUG] Doesn't start with nodejs 14.20.0 #2088

[sipopo]

I added warning message for minor check

[sipopo]

Hi, It is my first pull request in github. Do I need something to do else?

Sorry for my question...

[smortex]

#2101 was merged and update nodejs to 14.20.0, but the point of this PR/issue #2088 remains. I would suggest clarifying this by updating this Issue and PR subject to something like "Allow using a newer minor version of nodejs than the bundled one" or "Fix startup when using a newer minor version of nodejs then the bundled one" (since the issue is not 14.20.0 specific anymore).

At the bottom of the page, you will see 2 failing tests:

Click "Details" to see what was wrong, fix it and update the PR accordingly (see bellow for some hints). Each project is different, but the idea is usually to external contributors to work on their code and have automatic feedback, and reviewers will wait for all tests to be green before considering merging code. Until then, they may still of course help you.

You mention this is your first PR (awesome!), so it's good to know that any update you do in your branch will update this PR: you can re-write your commits and force push, and the PR will automagically update!

For example assuming you want to move your commits on top of master, sign them all to make the "Developer Certificate of Origin" check green, and rework them in a single commit (squash them), you can, from your working branch:

git fetch origin       # Download the latest code we have here
git rebase origin/main --signoff --interactive # Interactively move your commits on top of the main branch ensuring they are all signed

This will bring your editor listing all commits in your branch. Keep the first line unchanged, and replace pick with squash for all subsequent lines. Save and quit, your editor will pop again and allow you to combine all the commit messages in a single one.

Then push your updated branch:

git push -f            # Send the changes (-f is required because we re-wrote history)

[ashwin-pc]

@sipopo will you be able to add a signoff to your change? We cannot accept your change unless we have that. The PR otherwise looks great! @smortex's comment has detailed instructions on how to do so

[sipopo]

Sorry for late. I signed my commit.

[sipopo]

Sorry for my misunderstanding. I returned my changes to project )

[ashwin-pc]

@sipopo Thanks for making the change!

[ashwin-pc]

@sipopo Looks like your change is breaking a version check test.

[joshuarrrr]

We'll update this issue to make sure the community is clear on the change: https://github.com/opensearch-project/OpenSearch-Dashboards/issues/2088

But otherwise, @sipopo we agree with these changes and will plan to merge once the tests are updated.

@KrooshalUX Can you approve the wording of the warning.

[kgcreative]

We'll update this issue to make sure the community is clear on the change: #2088

But otherwise, @sipopo we agree with these changes and will plan to merge once the tests are updated.

@KrooshalUX Can you approve the wording of the warning.

Pulling the warning message out of the files for easier parsing:

var warnMessage =
    `OpenSearch Dashboards was built with ${requiredVersion}. ` +
    `The current Minor version of Node.js ${currentVersion} didn't test completely.`;
  console.warn(warnMessage);

Parsed with an example below: OpenSearch Dashboards was built with 14.20.0. The current Minor version of Node.js 14.20.1 didn't test completely.

The language of this feels a little unclear in terms of next steps and remediation. "Didn't test completely" doesn't tell me what went wrong or how to fix.

Consider: OpenSearch Dashboards requires NodeJS 14.20.0. The current version is 14.20.1

Can we get a little more clarity around what "Didn't test completely" means? Are there partial failures? Partial successes?

Perhaps Some tests failed due to a Node.js version mismatch. OpenSearch Dashboards has been tested to work with Node.js ${requiredVersion}. Your current version is ${currentVersion}.

[joshuarrrr]

Yeah, "didn't test completely" just means "this might work, but no guarantees".

Maybe

You're using an untested version of Node.js. OpenSearch Dashboards has been tested to work with Node.js ${requiredVersion}. Your current version is ${currentVersion}.

[kgcreative]

You're using an untested version of Node.js. OpenSearch Dashboards has been tested to work with Node.js ${requiredVersion}. Your current version is ${currentVersion}.

This works for me

[joshuarrrr]

@sipopo Just wanted to check-in to see if you're still able to update the tests.

[sipopo]

@joshuarrrr I did it :)

[joshuarrrr]

Thanks, @sipopo, I'll plan to take a look at the latest version on Monday!

[ashwin-pc]

@sipopo Thanks for the change, it looks good. The changelog check is failing because we introduced a changelog requirement for all PR's. Its a simple requirement. You just need to add an entry in the ./Changelog.md file for your change. Since this has been open for a while and since everything else looks good I'm okay merging it in as is and adding the changelog afterwards. But if possible, it would be ideal if the changelog entry happens in the same PR as the change itself for future reference.

[sipopo]

@ashwin-pc Could you give me text for Changelog.md, and I will add it. My english is not so perfect :)

[ashwin-pc]

You can add the title of the original issue

* Ability to start OS Dashboards with a newer and compatible nodejs version ([#2091](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2091))

You can add this under the Unreleased -> Features/Enhancements section

[codecov-commenter]

Codecov Report

Merging #2091 (41fa0a2) into main (9299784) will decrease coverage by 0.06%. The diff coverage is 71.42%.

@@            Coverage Diff             @@
##             main    #2091      +/-   ##
==========================================
- Coverage   66.65%   66.59%   -0.07%     
==========================================
  Files        3219     3219              
  Lines       61531    61536       +5     
  Branches     9431     9431              
==========================================
- Hits        41016    40981      -35     
- Misses      18277    18310      +33     
- Partials     2238     2245       +7     

Flag	Coverage Δ
Linux	66.59% <71.42%> (-0.01%)	:arrow_down:
Windows	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/setup_node_env/node_version_validator.js	68.75% <71.42%> (-3.98%)	:arrow_down:
src/dev/build/lib/get_build_number.ts	57.14% <0.00%> (-42.86%)	:arrow_down:
src/setup_node_env/harden/child_process.js	38.46% <0.00%> (-38.47%)	:arrow_down:
packages/osd-cross-platform/src/path.ts	51.21% <0.00%> (-34.15%)	:arrow_down:
...ges/osd-apm-config-loader/src/config.test.mocks.ts	91.30% <0.00%> (-8.70%)	:arrow_down:
src/dev/build/lib/config.ts	79.41% <0.00%> (-5.89%)	:arrow_down:
...s/osd-optimizer/src/node/node_auto_tranpilation.ts	83.67% <0.00%> (-4.09%)	:arrow_down:
packages/osd-optimizer/src/node/cache.ts	50.00% <0.00%> (-3.95%)	:arrow_down:
...ic/application/models/sense_editor/sense_editor.ts	64.00% <0.00%> (-0.89%)	:arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[ashwin-pc]

@kavilla @joshuarrrr Can you take a look at this? We can fix the conflict ourselves in github without dismissing the approvals.

[joshuarrrr]

@sipopo Will you be able to review @AMoo-Miki's suggestions and push updated changes?

[kavilla]

@sipopo Will you be able to review @AMoo-Miki's suggestions and push updated changes?

I think @sipopo has been playing ball and I think this we should move forward. Do we want to consider making the suggested changes and using @sipopo's author commits as a base to get them as a contributor?

[sipopo]

Hi, Are you waiting for me?

[ashwin-pc]

@sipopo yes, can you incorporate @AMoo-Miki 's changes in your PR manually? @kavilla was looking to see if someone could pick it up instead, but it looks like no one has the bandwidth right now.

[ashwin-pc]

So i pulled down the changes to validate if this works as expected only to run into a very interesting behavior. @sipopo @AMoo-Miki How did the two of you validate this functionality? Because when i run the yarn commands, it turns out that the first line of defense is node and package.json itself.

We have an engines key in the package.json file that fails first when we try to use any other minor or patch version of node. It does not even get to node check. The only two ways to get past this is to update the engine requirements to be something like ^14.20.0 or pass the --ignore-engines flag. Did either of you run into this while testing the feature? Also if you use the former option of specifying the node version using the caret symbol, the regex that you use is no longer valid. Here's one way to fix that:

1. Delete line 37 where we prefix v and rename rawRequiredVersion to requiredVersion. We know that this will not be null for our usecases so this is a safe assumption. And to be honest, i'm not sure that line makes any sense.
2. Modify the version regex to /^[v~=\^]+(\d+)\.(\d+)\.(\d+)/ to handle v, ^, ~ and = prefixes

With these changes, the rest of the code works as expected. I think this is also why @kavilla and @miki might have called out the initial change did not work.

[AMoo-Miki]

So i pulled down the changes to validate if this works as expected only to run into a very interesting behavior. @sipopo @AMoo-Miki How did the two of you validate this functionality? Because when i run the yarn commands, it turns out that the first line of defense is node and package.json itself.

The original idea was that this piece would relax the requirement and then engines.node would use 14.20. I felt that didn't eliminate the restrictions enough and hence I implemented this very differently in https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3402.

@sipopo I would have loved to add you as a co-author but due to the missing DCO in this PR, I couldn't. Thanks for all the effort you put in here; know that you were a big part of the motivation to get this done.