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

deploy on node 18 #9385

Merged
merged 7 commits into from
Aug 10, 2023
Merged

deploy on node 18 #9385

merged 7 commits into from
Aug 10, 2023

Conversation

chris48s
Copy link
Member

@chris48s chris48s commented Jul 15, 2023

Refs #9297
Refs #9323

Node 16 will reach EOL and stop getting security patches on 11 Sep 2023. I've been running my local dev copy with node 18 on and off recently and not noticed any problems. We've also been running the tests on node 18 in CI for ~a month

I would like to get #9323 reviewed and merged ahead of this one because once we upgrade to node 18, the default npm version is npm 9. Requiring contributors to use node 18 but requiring an old npm version and providing a v2 lockfile is a bit of a crap contributor experience. done + updated

Alternatively, I could smoosh those changes into this PR too.

TODO: update branch protection rules when we merge

  • 16 + 18 package lib tests have changed due to engine-strict true/false
  • remove main-18, integration-18, services-18
  • add main-20, integration-20, services-20

@chris48s chris48s added the operations Hosting, monitoring, and reliability for the production badge servers label Jul 15, 2023
@chris48s chris48s marked this pull request as draft July 15, 2023 13:08
@github-actions
Copy link
Contributor

github-actions bot commented Jul 15, 2023

Messages
📖 ✨ Thanks for your contribution to Shields, @chris48s!
📖

Thanks for contributing to our documentation. We ❤️ our documentarians!

Generated by 🚫 dangerJS against 2b735e3

@chris48s chris48s changed the title WIP deploy on node 18 deploy on node 18 Jul 15, 2023
@chris48s chris48s marked this pull request as ready for review July 15, 2023 13:27
@github-actions
Copy link
Contributor

🚀 Updated review app: https://pr-9385-badges-shields.fly.dev

@github-actions
Copy link
Contributor

🚀 Updated review app: https://pr-9385-badges-shields.fly.dev

Copy link
Member

@PyvesB PyvesB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still uses Node 16, shall we update it as well whilst we're at it?

.github/workflows/test-integration-20.yml Show resolved Hide resolved
.github/workflows/test-services-20.yml Show resolved Hide resolved
Copy link
Collaborator

@jNullj jNullj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't badge-maker/package.json also get updated to node 18?

@chris48s
Copy link
Member Author

chris48s commented Aug 8, 2023

This still uses Node 16, shall we update it as well whilst we're at it?

node16 is the highest supported runner for javascript actions. The next jump will be to 20. See actions/runner#2704

Fortunately, keeping this in sync with the rest of the codebase doesn't really matter as none of the application code is run by this action. Only the files in https://github.com/badges/shields/tree/master/.github/actions/close-bot

Shouldn't badge-maker/package.json also get updated to node 18?

Badge-maker is a package which is used by other applications as well as shields.io, so we try to test against a fairly wide range of node versions (that range needs to include whatever major node version we deploy shields.io on, but ideally it is as wide as we can make it without jumping through too many hoops). At the moment, badge-maker is tested on Node 16, 18 and 20. There's no need to drop compatibility with node 16 for now so >=16 is the right range.

Copy link
Member

@PyvesB PyvesB 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 to me! 👍🏻

@chris48s chris48s added this pull request to the merge queue Aug 10, 2023
Merged via the queue into badges:master with commit 0ffa366 Aug 10, 2023
20 checks passed
@chris48s chris48s deleted the 9297-node18 branch August 10, 2023 18:49
@chris48s chris48s mentioned this pull request Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
operations Hosting, monitoring, and reliability for the production badge servers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants