s2i-nodejs-container icon indicating copy to clipboard operation
s2i-nodejs-container copied to clipboard
verified publisher icon sclorg

fix: specifying nodejs version to be on 18

Open pacostas opened this issue 3 years ago • 5 comments

This fix, specifies nodejs version to be on 18, otherwise the build image does not build as described on this issue https://github.com/sclorg/s2i-nodejs-container/issues/357

pacostas avatar Sep 21 '22 17:09 pacostas

Makes sense to me, seems to be missing compared to other files.

mhdawson avatar Sep 23 '22 20:09 mhdawson

My guess is that the assumption might have been that Node.js 18 was the default for RHEL 9, but it looks like that is not the case. @kasicka does it make sense to you that the default is Node.js 16 ?

mhdawson avatar Sep 23 '22 20:09 mhdawson

It's just a leftover from copying the Dockerfile of the 16 version (which does not use modules) and not making necessary modifications for module use. LGTM. One thing though - @pacostas can you update 18/Dockerfile.rhel9 as well with the same change? Note that the images are not yet set up to be built as part of the CI, so running it makes no sense now. Let's figure out the issues (if any) once we start officially building the images.

pkubatrh avatar Sep 26 '22 08:09 pkubatrh

@pkubatrh From what I see

  • On rhel9/ubi9 node 18 is not available for installing it ATM. Will be available as a module on the next rhel9.1 release, so it might be necessary to specify the nodejs version as on centos 8 stream and centos 9 stream using the command yum -y module enable nodejs:$NODEJS_VERSION
  • On rhel8/ubi8 node 18 is not available yet, will be available on rhel8.7 version
  • On centos-9-stream, node 18 is available but needs to specify nodejs version [This PR specifies nodejs version]
  • On centos-8-stream, node 18 is available and also specified. Image works.
  • On fedora , node 18 is available and also specified. Image works.

Updating the PR, specifying also NODEJS_VERSION and on rhel9.1. For rhel8 the version of nodejs is already specified.

pacostas avatar Sep 29 '22 13:09 pacostas

For the minimal images. Added also specifying the nodejs version on rhel9 images. Same pattern as on rhel8, centos 8 stream & centos 9 stream.

pacostas avatar Sep 29 '22 14:09 pacostas

[test-all]

pkubatrh avatar Oct 12 '22 08:10 pkubatrh

Above testing does not cover the changes. I guess we have to wait for an additional commit which will include the c9s, rhel9, rhel9 - minimal. @pkubatrh

pacostas avatar Oct 12 '22 10:10 pacostas

The NodeJS 18 testing is covered by #362

phracek avatar Oct 12 '22 12:10 phracek