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

[OSD][Build] nvm use #1905

Merged
merged 5 commits into from
Apr 8, 2022
Merged

Conversation

kavilla
Copy link
Member

@kavilla kavilla commented Apr 5, 2022

Description

Within OSD we have a .nvmrc file that is used when nvm use
is executing. So we can use the right node version for the
build.

Issue:
n/a

Signed-off-by: Kawika Avilla [email protected]

Issues Resolved

n/a

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Within OSD we have a .nvmrc file that is used when `nvm use`
is executing. So we can use the right node version for the
build.

Issue:
n/a

Signed-off-by: Kawika Avilla <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Apr 5, 2022

Codecov Report

Merging #1905 (bb6daa2) into main (0f02c6f) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #1905   +/-   ##
=========================================
  Coverage     94.60%   94.60%           
  Complexity       20       20           
=========================================
  Files           178      178           
  Lines          3633     3633           
  Branches         27       27           
=========================================
  Hits           3437     3437           
  Misses          192      192           
  Partials          4        4           

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 0f02c6f...bb6daa2. Read the comment docs.

@kavilla
Copy link
Member Author

kavilla commented Apr 5, 2022

Jenkins is doing stuff that is preventing us from passing the environment variable and running the command that was added https://issues.jenkins.io/browse/JENKINS-51307. Therefore the current node version is being used and having some failures while building 2.0.0.

This also doesn't work because nvm isn't available but I think we need to add nvm to the PATH and have the docker image published so that it can be used here: https://github.com/opensearch-project/opensearch-build/blob/main/docker/ci/dockerfiles/build.centos7.opensearch-dashboards.x64.arm64.dockerfile#L151

@peterzhuamazon
Copy link
Member

peterzhuamazon commented Apr 5, 2022

Jenkins is doing stuff that is preventing us from passing the environment variable and running the command that was added https://issues.jenkins.io/browse/JENKINS-51307. Therefore the current node version is being used and having some failures while building 2.0.0.

This also doesn't work because nvm isn't available but I think we need to add nvm to the PATH and have the docker image published so that it can be used here: https://github.com/opensearch-project/opensearch-build/blob/main/docker/ci/dockerfiles/build.centos7.opensearch-dashboards.x64.arm64.dockerfile#L151

This does work if you can source the nvm.sh before running nvm.

source $NVM_DIR/nvm.sh

@kavilla
Copy link
Member Author

kavilla commented Apr 5, 2022

Jenkins is doing stuff that is preventing us from passing the environment variable and running the command that was added https://issues.jenkins.io/browse/JENKINS-51307. Therefore the current node version is being used and having some failures while building 2.0.0.
This also doesn't work because nvm isn't available but I think we need to add nvm to the PATH and have the docker image published so that it can be used here: https://github.com/opensearch-project/opensearch-build/blob/main/docker/ci/dockerfiles/build.centos7.opensearch-dashboards.x64.arm64.dockerfile#L151

This does work if you can source the nvm.sh before running nvm.

source $NVM_DIR/nvm.sh

I understand the NVM docs call out to define $NVM_DIR when setting it up but I'm not seeing the preference to source a script from an environment variable that may not exist rather than adding it to the PATH for the docker image. Would seem like an anti pattern for the repo as well.

@kavilla
Copy link
Member Author

kavilla commented Apr 5, 2022

If I source $NVM_DIR will make sure to update https://github.com/opensearch-project/opensearch-build/blob/main/DEVELOPER_GUIDE.md#nvm-and-node to ensure users define it in the environment.

Signed-off-by: Kawika Avilla <[email protected]>
Signed-off-by: Kawika Avilla <[email protected]>
@kavilla kavilla marked this pull request as ready for review April 7, 2022 11:44
@kavilla kavilla requested a review from a team as a code owner April 7, 2022 11:44
@kavilla
Copy link
Member Author

kavilla commented Apr 7, 2022

The builds for 2.0.0 have been broken for a couple days now so to unblock this I have created a follow up issue: #1922 since I still believe adding to PATH fits into this repo more.

Signed-off-by: Kawika Avilla <[email protected]>
Signed-off-by: Kawika Avilla <[email protected]>
Comment on lines +112 to +114
echo "Setting node version"
source $NVM_DIR/nvm.sh
nvm use
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a check if $NVM_DIR isn't defined then send them to the setup guide - this would be a good fast follow

Copy link
Member Author

@kavilla kavilla Apr 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes that would actually be a really good solution that will make me feel better about this code being in here. I've created an issue #1922 but trying my best not to be online so I might not be able to get this as a fast follow.

@peterzhuamazon peterzhuamazon merged commit 1116d3b into opensearch-project:main Apr 8, 2022
kavilla added a commit to kavilla/opensearch-build that referenced this pull request Apr 8, 2022
Origin: opensearch-project#1905

Incorrectly copied the path while fixing the build to use NVM.

Issue:
n/a

Signed-off-by: Kawika Avilla <[email protected]>
@kavilla kavilla mentioned this pull request Apr 8, 2022
1 task
kavilla added a commit that referenced this pull request Apr 8, 2022
Origin: #1905

Incorrectly copied the path while fixing the build to use NVM.

Issue:
n/a

Signed-off-by: Kawika Avilla <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants