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

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

Closed
dblock opened this issue Feb 7, 2022 · 11 comments · Fixed by #3508
Closed

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

dblock opened this issue Feb 7, 2022 · 11 comments · Fixed by #3508
Assignees
Labels
enhancement New feature or request

Comments

@dblock
Copy link
Member

dblock commented Feb 7, 2022

Is your feature request related to a problem? Please describe.

Similar to opensearch-project/OpenSearch#1872, add support for a OPENSEARCH_DASHBOARDS_NODE_HOME or OSD_NODE_HOME.

A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]

This would enable a similar affordance to OpenSearch to configure an external version of node.

Additional context

I am coming from opensearch-project/OpenSearch#2001 (comment).

@dblock dblock added the enhancement New feature or request label Feb 7, 2022
@dblock
Copy link
Member Author

dblock commented Feb 7, 2022

I am coming from opensearch-project/OpenSearch#2001 (comment), but I am not convinced that users want to use an external version of node.

@dblock
Copy link
Member Author

dblock commented Feb 7, 2022

@jcgraybill Do you have links to any mentions of users wanting to run Dashboards with a different/system version of node than the one bundled?

@jcgraybill
Copy link

This is already possible - see bin/opensearch-dashboards:

if [ -x "${DIR}/node/bin/node" ]; then
  NODE="${DIR}/node/bin/node"
else
  NODE="$(which node)"
fi

I thought we were just discussing how this will work. Are you looking to make a case that it should be removed altogether?

@dblock
Copy link
Member Author

dblock commented Feb 7, 2022

This is already possible - see bin/opensearch-dashboards:
I thought we were just discussing how this will work. Are you looking to make a case that it should be removed altogether?

I am not saying that we should remove the code above.

This code makes it possible to use a specific version of node by removing node from the distribution, and then adding it to PATH. It feels like an (undocumented) side effect as opposed to a feature that lets me deliberately state which node.js I'd like to use. Most use-cases seem to want to deliberately switch from one to another, especially when there are multiple versions of node installed on the system. So this feature request is to discuss adding the environment variable that lets me control what node.js to use without any ambiguity.

@jcgraybill
Copy link

Nb this PR #1189

@boktorbb
Copy link
Contributor

boktorbb commented Feb 8, 2022

I'm not sure I fully follow what the ask is here @dblock. We use the .nvmrc file that explicitly contains the node version being used. If you do nvm use it makes use of that node version moving forward. If you don't have it installed, it will let you know. This seems to me to be a non-ambiguous way of specifying the node version. Let me know if I'm misunderstanding here

@dblock
Copy link
Member Author

dblock commented Feb 9, 2022

@boktorbb-amzn To avoid any ambiguity, I am talking about users installing OpenSearch Dashboards, not during development. Is .nvmrc the documented way to switch node.js runtime for end-user installations? Is it documented?

A bit more background. Many users don't use the bundled version of Java in OpenSearch, but use the globally provided one. Thus OpenSearch distributes a no-JDK version and officially supports multiple versions of Java. Does this also happen for OpenSearch Dashboards wrt node.js?

Note #1219 (comment) which shows that Dashboards already supports a global version of node.js, but if you're upgrading this is not helpful.

@kavilla
Copy link
Member

kavilla commented Feb 9, 2022

Does this also happen for OpenSearch Dashboards wrt node.js?

I don't have data to support that this or isn't a common setup for many users. Original when that code was introduced I believe it was due to the build not correctly bundling Node for that specific operating system so then it would default to using the Node within the system. Nothing does stop from deleting the Node folder so then it defaults to using the global version of Node.

We could support this because it makes life easier but I do think it should be documented that the code was developed, built, and verified it was done with a specific version of Node. So this setting could be used but have some consequences if there were any API changes. However, even if this variable is set, should we still prevent them from running if they try to run any node version that isn't just a different in patch versions?

@dblock
Copy link
Member Author

dblock commented Feb 14, 2022

Looks like #1189 is timely and awesome.

@kavilla kavilla linked a pull request Aug 31, 2022 that will close this issue
7 tasks
@AbhishekReddy1127 AbhishekReddy1127 added the v2.5.0 'Issues and PRs related to version v2.5.0' label Dec 15, 2022
@joshuarrrr joshuarrrr added v2.6.0 and removed v2.5.0 'Issues and PRs related to version v2.5.0' labels Jan 9, 2023
@joshuarrrr
Copy link
Member

bumping to 2.6.0 release target.

@AMoo-Miki
Copy link
Collaborator

AMoo-Miki commented Feb 9, 2023

I am not sure I understand the motivation behind this. I would support adding NODE_HOME support to the shell/batch scripts but I can't find a reason for not doing that and instead doing OSD_NODE_HOME.

NODE_HOME, or *_HOME, is a common practice and if someone wants to use a specific runtime binary, they could just do NODE_HOME=/opts/nodejs16 bin/opensearch-dashboards.

Am I missing something?

PS, I feel the OpenSearch issue was that they had the variable erroneously documented and instead of revoking the docs, they decided to honor it. If this is purely for consistency, i could support it... reluctantly; Dashboards never had NODE_HOME so this area was never consistent and IMO, need not be made consistent either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
7 participants