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.

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

Closed
wants to merge 1 commit into from

Conversation

sheplu
Copy link
Member

@sheplu sheplu commented May 10, 2023

Update Matrix with Node.js version

  • update v16
  • update v18
  • update v19
  • add v20
  • add v21

@sheplu sheplu force-pushed the matrix-nodejs-20 branch from f7e9bd3 to 7b59a5a Compare May 10, 2023 21:42
@sheplu sheplu marked this pull request as ready for review May 10, 2023 21:48
Copy link

@rubiin rubiin left a 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

Copy link

@rubiin rubiin left a 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

@sheplu sheplu force-pushed the matrix-nodejs-20 branch 3 times, most recently from 85a1ff2 to 8dc9cd0 Compare August 17, 2023 15:53
@sheplu sheplu requested a review from dougwilson August 17, 2023 15:57
@sheplu sheplu force-pushed the matrix-nodejs-20 branch 2 times, most recently from 1d27450 to b282698 Compare October 11, 2023 07:03
@sheplu
Copy link
Member Author

sheplu commented Oct 29, 2023

@dougwilson I can't manually retrigger all the tests but it should be working fine for all versions

Copy link
Member

@UlisesGascon UlisesGascon left a 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

Copy link
Member

@aravindvnair99 aravindvnair99 left a 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?

@dougwilson
Copy link
Contributor

dougwilson commented Nov 11, 2023

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.

@sheplu
Copy link
Member Author

sheplu commented Feb 24, 2024

Closing as this will land part of #5429 and #5430

@sheplu sheplu closed this Feb 24, 2024
@sheplu sheplu deleted the matrix-nodejs-20 branch February 24, 2024 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants