fix(docker): bumped up node version #7822
Conversation
| @@ -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 | |||
There was a problem hiding this comment.
Hasn't this just been upgraded here? #7782 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I have pushed a change were NodeJS version now is global.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Alright, no problem! i have reverted it back now to the previous change :)
There was a problem hiding this comment.
Would you mind mentioning in the commit message that this was a left over from: cad94e3 ?
There was a problem hiding this comment.
I have added it in the commit message
There was a problem hiding this comment.
With the current docker implementation, it should change only in one place, .versions
ARG will be overridden
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7822 +/- ##
=========================================
Coverage 67.06% 67.06%
Complexity 2041 2041
=========================================
Files 356 356
Lines 17045 17045
Branches 2438 2438
=========================================
Hits 11431 11431
Misses 4594 4594
Partials 1020 1020
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
afilekovic
force-pushed
the
main
branch
3 times, most recently
from
8a929cd
to
bebb377
Compare
|
@afilekovic there's now a merge commit in your PR, which we don't allow. Please drop that and rebase against latest |
afilekovic
force-pushed
the
main
branch
2 times, most recently
from
14b4b7e
to
cc25486
Compare
This is a fixup for cad94e3. Signed-off-by: Armin Filekovic <armin.filekovic@ingka.com>
|
Merging despite the current test failures, which are not caused by this PR. |
I tried to run ort analyze but I got the error that npm could not be found. I found in the dockerfile that NODEJS_VERSIONS differed between the installed version and the used version(18.14.2 and 20.9.0). This caused ort to not be able to run properly. After changing the NODEJS_VERSIONS to 20.9.0 ort scan started to work since the old version(18.14.2) was not installed and it could not be found.