Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENHANCEMENT] Ability to start OS Dashboards with a newer and compatible nodejs version #2088

Closed
sipopo opened this issue Aug 8, 2022 · 8 comments · Fixed by #3402
Closed
Labels
discuss enhancement New feature or request v2.6.0

Comments

@sipopo
Copy link

sipopo commented Aug 8, 2022

Describe the bug

The application doesn't start with nodejs version 14.20.0

To Reproduce
Upgrade nodejs to version 14.20.0 and run the application.

Expected behavior
The application should start.

OpenSearch Version
OpenSearch-Dashboard 2.1.0

Additional context
Errors in log: OpenSearch Dashboards was built with v14.19.1 and does not support the current Node.js version v14.20.0. Please use Node.js v14.19.1 or a higher patch version.

@sipopo sipopo added bug Something isn't working untriaged labels Aug 8, 2022
@sipopo sipopo changed the title [BUG] Doesn't start with nodejs 14.12.0 [BUG] Doesn't start with nodejs 14.20.0 Aug 8, 2022
sipopo added a commit to sipopo/OpenSearch-Dashboards that referenced this issue Aug 8, 2022
@kavilla
Copy link
Member

kavilla commented Aug 8, 2022

Hello @sipopo,

Thanks for opening. I would be hesitant to call this a bug because it's quite intentional. As indicated by the message, OpenSearch Dashboards was built with 14.19 and we are semi-confident that there won't be any unforeseen issues by having different patch versions. But that risk increases even more when we allow for minor versions to be different.

In my opinion I'd think it's more safe to leave this behavior the same. At this point I'd be more comfortable with this being implemented: #1219 and have a warning message about using a higher version.

But I would like to hear from the other @opensearch-project/opensearch-dashboards-core.

@ashwin-pc
Copy link
Member

I'm curious to know if we have any documented examples of using higher minor versions of node breaking anything in OSD. The entire purpose of semver is to introduce non breaking backward compatible changes in minor version bumps (And node does follow semver). To not use that and lock down the major, minor and patch versions of NodeJS used seems over cautious imo.

Like you called out, i'd be open to compromising and adding a warning to higher minor versions, but I'd like to hear some arguments about why we'd even need that.

@smortex
Copy link

smortex commented Aug 9, 2022

👍 for honoring SemVer: currently we actively reject expected to be compatible versions. Also if a security issue is found in node 14, it would be fixed as 14.20.1 (now that 14.20.0 has been released), this mean that the fixed version could not be used without prior patching.

@smortex
Copy link

smortex commented Aug 9, 2022

Also if a security issue is found in node 14, it would be fixed as 14.20.1

My bad, 14.20.0 already fix three medium severity issues and two high severity issues according to https://nodejs.org/en/blog/vulnerability/july-2022-security-releases/ 🙃

@kavilla
Copy link
Member

kavilla commented Aug 9, 2022

I'm curious to know if we have any documented examples of using higher minor versions of node breaking anything in OSD. The entire purpose of semver is to introduce non breaking backward compatible changes in minor version bumps (And node does follow semver). To not use that and lock down the major, minor and patch versions of NodeJS used seems over cautious imo.

Like you called out, i'd be open to compromising and adding a warning to higher minor versions, but I'd like to hear some arguments about why we'd even need that.

I think that's one of the issues that we still have not done enough research in why this check was required and carried over from the fork. The original expected to use the bundled Node version and if the Node executable isn't available it will default to use the env global version, and expected the same version.

Historically I can only find that the reason why this logic existed was that they said Node adheres to semver but the legacy app said they can't guarantee it Node does without bumping the application version as well.

I still think in the end the overall goal should match what OpenSearch is doing with OPENSEARCH_JAVA_HOME, where there's a check but basically setting env variable path will override the checking of the pre-bundled Java regardless if the file is there or not.

At which point, this will solve what @smortex is referencing because the vulnerabilities would still exist if we built the application with Node 14.20.0 and folks don't manually delete the pre-bundled Node.

ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this issue Aug 9, 2022
@ashwin-pc
Copy link
Member

I still think in the end the overall goal should match what OpenSearch is doing with OPENSEARCH_JAVA_HOME, where there's a check but basically setting env variable path will override the checking of the pre-bundled Java regardless if the file is there or not.

I think this is a separate conversation

At which point, this will solve what @smortex is referencing because the vulnerabilities would still exist if we built the application with Node 14.20.0 and folks don't manually delete the pre-bundled Node.

I think the ask here is much simpler. It's just to allow the user to use minor versions greater than the one defined in .nvmrc. The bundled node versions will always contain the version we specify, but if a user has a higher node minor version on their system, we dont need to fail the build, but instead just give a warning. We already do the same thing with our npm dependencies when the minor versions dont match.

@smortex
Copy link

smortex commented Aug 12, 2022

#2101 was merged and update nodejs to 14.20.0 so this Issue title is not true anymore. I suggest to update it to better match the discussion, something like "[BUG] Not able to start OS Dashboards with a newer and compatible nodejs version"

@joshuarrrr joshuarrrr changed the title [BUG] Doesn't start with nodejs 14.20.0 [ENHANCEMENT] Allow for later minor version differences in node Oct 4, 2022
@joshuarrrr
Copy link
Member

Updated the issue title.

@joshuarrrr joshuarrrr changed the title [ENHANCEMENT] Allow for later minor version differences in node [ENHANCEMENT] Ability to start OS Dashboards with a newer and compatible nodejs version Oct 4, 2022
@joshuarrrr joshuarrrr added enhancement New feature or request and removed bug Something isn't working labels Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss enhancement New feature or request v2.6.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants