theforeman
Fixes #37134 - upgrade to node 18 and npm 8
For context, this is blocking EL9 so if we can't get this done for Foreman 3.10 we'll need to postpone that effort to 3.11. I'd really like to avoid that.
If webpack 5 is not reverted I dont see a reason for this to not be merged really soon. Looking at the test failures now.
@ekohl do we need npm8 for EL9?
Most likely In EL9 they've packaged NPM 8 and we're not downgrading that to 6. So if we need NPM (and I'm pretty sure we do), we need to be on NPM 8. It is acceptable to also move our other platforms to NodeJS 16 and NPM 8 if needed (and I think we can't support both).
However, looking at the Foreman 3.10 schedule we'll have stabilization week from 2024-02-12 - 2024-02-17. The week before we have cfgmgmtcamp where both @evgeni and I will be.
Essentially that means we need to get this and the needed rebuilds done next week. I'm skeptical we can make that work, unless @ehelms steps in. Even then, getting reviews will be hard.
Yeah, npm8 is what we get via CentOS packages and we can't easily avoid that.
For clarity sake, on EL 8, we get the following:
Installing:
nodejs x86_64 1:16.18.1-3.module_el8.7.0+1234+1d8589d9 appstream 12 M
Installing dependencies:
brotli x86_64 1.0.6-3.el8 baseos 323 k
openssl x86_64 1:1.1.1k-9.el8 baseos 737 k
Installing weak dependencies:
nodejs-docs noarch 1:16.18.1-3.module_el8.7.0+1234+1d8589d9 appstream 9.3 M
nodejs-full-i18n x86_64 1:16.18.1-3.module_el8.7.0+1234+1d8589d9 appstream 8.0 M
npm x86_64 1:8.19.2-1.16.18.1.3.module_el8.7.0+1234+1d8589d9 appstream 2.0 M
If this is just a question of iterating over packages for rebuilds, I can certainly help grind through that. I would need more clarity on if https://github.com/theforeman/foreman-js/pull/404 is required and if that's the only property affected by this change? IIRC we do not store the lock files for any other repository.
When I tried I could actually install it without forcing legacy peer dependencies. Are you sure it's still needed?
did you run npm test or compile webpack? for me tests were failing as they could not find test packages, and pf5 was chosen over pf4
did you run
npm testor compile webpack? for me tests were failing as they could not find test packages, and pf5 was chosen over pf4
No, I didn't. I just assumed that the initial error (plainly refusing to install at all) was the only error.
Just to be clear, I'm not opposed to using it for now if it gets us on NodeJS 16 faster.
We discussed this and I think @evgeni, @ehelms and I have no objection to jumping to NodeJS 18. A major benefit IMHO is that at least on Fedora 38 you can use the system NodeJS again which makes developer's lives easier (f39 has NodeJS 20, so supporting that too would be nice but not an immediate goal).
Things look to work at node 20 as well.
Should we limit node version in package.json? the things that dont work right now with node 16 and down is only the snapshot tests I think.
Also checking test failures.
I'm wondering if we should choose NodeJS 18 or 20 as our default. Do you expect problems with NodeJS 20?
I opened up a bunch of PRs to test with this:
- https://github.com/theforeman/foreman-tasks/pull/742
- https://github.com/theforeman/foreman_ansible/pull/693
- https://github.com/theforeman/foreman_bootdisk/pull/157
- https://github.com/theforeman/foreman_discovery/pull/621
- https://github.com/theforeman/foreman_google/pull/68
- https://github.com/theforeman/foreman_leapp/pull/138
- https://github.com/theforeman/foreman_openscap/pull/558
- https://github.com/theforeman/foreman_puppet/pull/385
- https://github.com/theforeman/foreman_remote_execution/pull/872
- https://github.com/theforeman/foreman_webhooks/pull/73
I did mess up my scripting and opened them from my own master branch, but I'm first going to see what those tests bring.
I don't expect problems, and also put ">=18.0.0 <21.0.0" in the package.json limits
Can we keep it at >=14 for now? I don't think we want to switch EL8 right now.
https://github.com/theforeman/foreman_ansible/pull/693#issuecomment-1934237215 and https://github.com/theforeman/foreman_discovery/pull/621 look related, the rest of the failures look unrelated.
Can we keep it at >=14 for now? I don't think we want to switch EL8 right now.
Are we ok with the js tests failing for node 14?
theforeman/foreman_ansible#693 (comment) and theforeman/foreman_discovery#621 look related, the rest of the failures look unrelated.
https://github.com/theforeman/foreman/pull/10045 should resolve those. They also happen with current NodeJS.
All the plugin tests now pass. That means there's only the NodeJS 14 situation left.
We plan on shipping EL8 with NodeJS 14 for Foreman 3.10 so it will be nice to testing that. Otherwise you may pull in dependencies that are Node 14 incompatible.
That makes me wonder: why is this snapshot changing? Are there also effects on the end user because of this? If not, what is the test really proving?
That makes me wonder: why is this snapshot changing? Are there also effects on the end user because of this? If not, what is the test really proving?
Node has changes how dates are in strings, so instead of , there is at now, but the date is the same and thats what we test.
Digging a bit deeper, it's https://github.com/nodejs/node/issues/45945 but I think at runtime toLocaleString uses the browser API, not NodeJS. So it doesn't really matter for end users. Though it does matter for our tests.
Is there a way to provide a NodeJS version dependent snapshot? Would that make sense?
We can probably do that with setting different paths for snapshots for different versions. Are we planning to test and actively support node 14 or just leave it as an option?
Are we planning to test and actively support node 14 or just leave it as an option?
As I said in https://github.com/theforeman/foreman/pull/10016#issuecomment-1946594821, we'll have it at least for Foreman 3.10 so I'd prefer to have testing for it too.
Waiting for the container build to finish, but I think this is ready to merge now.
@evgeni we should pick this to 3.10 too, right?
Given we publish EL9 packages that use NodeJS 18 it would be formally correct to do that, yes.