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.

Sign up for GitHub

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

fix(docker): bumped up node version #7822

Merged
merged 1 commit into from
Nov 14, 2023
Merged

fix(docker): bumped up node version #7822

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,7 @@ COPY --from=python --chown=$USER:$USER $PYENV_ROOT $PYENV_ROOT
RUN syft $PYENV_ROOT -o spdx-json --file /usr/share/doc/ort/ort-python.spdx.json

# NodeJS
ARG NODEJS_VERSION=18.14.2
ARG NODEJS_VERSION=20.9.0
Copy link
Member

Choose a reason for hiding this comment

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

Hasn't this just been upgraded here? #7782 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the dockerfile there are 2 references from what i can see. In #7782 it has been changed in the nodejsbuild on line 184. Then there is a reference in the run step at line 458 that has not been changed.

Copy link
Member

@sschuberth sschuberth Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, it's a bit unfortunate that this argument is repeated in two stages. I guess the correct way to solve this is to make it a "global" argument which gets its default value only assigned once, and only repeat the argument's name (but not its default value) in the stages that use the argument. But that aside, I believe the fix is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have pushed a change were NodeJS version now is global.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, but if doing that in this PR, it IMO should be done consistently for all tool versions (even if they might not be used in different stages yet, but as a preparation for being able to do so).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. Maybe it's indeed better to just stick to the original commit in this PR to change the version, and do the rest later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, no problem! i have reverted it back now to the previous change :)

Copy link
Member

Choose a reason for hiding this comment

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

Would you mind mentioning in the commit message that this was a left over from: cad94e3 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added it in the commit message

Copy link
Contributor

Choose a reason for hiding this comment

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

With the current docker implementation, it should change only in one place, .versions
ARG will be overridden

ENV NVM_DIR=/opt/nvm
ENV PATH=$PATH:$NVM_DIR/versions/node/v$NODEJS_VERSION/bin
COPY --from=nodejs --chown=$USER:$USER $NVM_DIR $NVM_DIR
Expand Down
Loading