-
-
Notifications
You must be signed in to change notification settings - Fork 16.8k
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
update Node.js version 16.x, 18.x, 19.x and add 20.x #5190
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update to the latest lts which is 18.17.1 as it fixes some CVEs
75b42cc
to
4f468e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Also I was wondering why are we running tests for all available node versions. Most of them (below 14) have stopped receiving any kind of security updates and its most likely anyone is using them.
Removal of the prior deprecated versions would help speeding up the CI
85a1ff2
to
8dc9cd0
Compare
8dc9cd0
to
f519ad9
Compare
1d27450
to
b282698
Compare
b282698
to
a0d885f
Compare
a0d885f
to
b22feb6
Compare
@dougwilson I can't manually retrigger all the tests but it should be working fine for all versions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Seems like the CI issue can be resolved by re-running the CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something to consider - Do we really need to hardcode the minor values?
We originally didn't have them, but Node.js would keep releassing versions that would break our CI in the minor. Then CITGM was made to fix that, but I think they removed express so probably even more useful to keep them pinned still. It really stinks when ever people try and make PRs or we try to do releases when the CI is suddenly broken and then the time needs to be spent fixing that instead. I think most recent the CI broke in a minor 18.x release of Node.js bc they major bumped npm. Thankfully didn't break CI when that happened so could fix when the focus was on updating CI instead of on something else. It does have the added benefit too that bumping the minor forces a retest on that new Node.js, otherwise it will sit there until we change something unrelated. |
Update Matrix with Node.js version