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

change to node20 for all actions #2006

Merged
merged 16 commits into from
Dec 13, 2023
Merged

change to node20 for all actions #2006

merged 16 commits into from
Dec 13, 2023

Conversation

nickfyson
Copy link
Contributor

@nickfyson nickfyson commented Nov 22, 2023

This PR upgrades all the actions to use node20, and bumps the major version to v3.

It also adds a new PR check to confirm the consistency of node versions across all actions, and ensure that we do not change this version in backport PRs to older releases.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@nickfyson nickfyson added the Update dependencies Trigger PR workflow to update dependencies label Nov 22, 2023
@github-actions github-actions bot removed the Update dependencies Trigger PR workflow to update dependencies label Nov 22, 2023
Copy link
Contributor

Pushed a commit to update the checked-in dependencies. Please mark the PR as ready for review to trigger PR checks.

@nickfyson nickfyson marked this pull request as ready for review November 22, 2023 10:56
@nickfyson nickfyson requested a review from a team as a code owner November 22, 2023 10:56
@nickfyson nickfyson marked this pull request as draft November 22, 2023 10:56
Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

I think the changes you made are good for bumping the node version of the actions. However, once we merge this, we can no longer release v2 of the action until we also update the release scripts to what we had before when we released v1 and v2 (now will be v2 and v3). That's a dicey state to be in.

So, either we can recreate the release scripts and merge it first, or we do them at the same time. I can't remember how we did it last time.

CHANGELOG.md Outdated Show resolved Hide resolved
foobar.txt Outdated Show resolved Hide resolved
@aeisenberg
Copy link
Contributor

Right...it was this PR that added the v2->v1->main release steps #995. It was merged before the node version changes were merged in #1000.

@aeisenberg
Copy link
Contributor

This PR #1482 is the revert PR that removed the v1 release process.

@nickfyson nickfyson added the Update dependencies Trigger PR workflow to update dependencies label Dec 7, 2023
@github-actions github-actions bot removed the Update dependencies Trigger PR workflow to update dependencies label Dec 7, 2023
Copy link
Contributor

github-actions bot commented Dec 7, 2023

Pushed a commit to update the checked-in dependencies. Please mark the PR as ready for review to trigger PR checks.

@nickfyson nickfyson marked this pull request as ready for review December 7, 2023 16:24
@nickfyson nickfyson marked this pull request as draft December 7, 2023 17:39
@nickfyson nickfyson marked this pull request as ready for review December 7, 2023 17:39
Copy link
Contributor

@henrymercer henrymercer 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. Let's wait for the CodeQL 2.15.4 release to complete before merging this to reduce the risk of that release.

One potential safety check we could add is a PR check that fails if the release branch is v2 and the Node version of any Actions is Node 20. This could help make sure (a) we don't forget to switch the v2 branch back to running on node16 the first time we release it, and (b) any new Action we add to main runs on node16 in the v2 branch.

henrymercer
henrymercer previously approved these changes Dec 13, 2023
.github/workflows/pr-checks.yml Outdated Show resolved Hide resolved
.github/actions/check-sarif/action.yml Outdated Show resolved Hide resolved
henrymercer
henrymercer previously approved these changes Dec 13, 2023
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

Minor suggestions to the new PR check, looks good overall :)

.github/workflows/pr-checks.yml Outdated Show resolved Hide resolved
.github/workflows/pr-checks.yml Outdated Show resolved Hide resolved
.github/workflows/pr-checks.yml Outdated Show resolved Hide resolved
.github/workflows/pr-checks.yml Outdated Show resolved Hide resolved
.github/workflows/pr-checks.yml Outdated Show resolved Hide resolved
.github/workflows/pr-checks.yml Outdated Show resolved Hide resolved
@nickfyson
Copy link
Contributor Author

NB – The introduction of a matrix for the Check JS job means that Check JS has been replaced by Check JS (16.11) and Check JS (current).

Once this PR is approved and ready to merge I will remove Check JS as a required check, and once merged I will run the update-required-checks.sh script in order to make the new checks required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants