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

Run action with Node 18 #60

Merged
merged 2 commits into from
Jul 26, 2023
Merged

Run action with Node 18 #60

merged 2 commits into from
Jul 26, 2023

Conversation

georgeblahblah
Copy link
Contributor

@georgeblahblah georgeblahblah commented Jul 26, 2023

Why are you making this change?

During the migration work to build Frontend with GitHub Actions instead of TeamCity (guardian/frontend#26058), artifact upload using actions-riff-raff would sometimes fail. The failure is caused by a known issue in @aws-sdk/client-s3, which is fixed from Node 18.6.0.

Currently, actions-riff-raff is a Javascript action. Javascript actions only support Node 12 and Node 16 runtimes and GitHub are not planning to support Node 18, instead planning support for Node 20 at some point later this year (actions/runner#2704).

Given the upcoming deprecation of TeamCity and the bug in uploading artifacts to S3 with @aws-sdk/client-s3 in Node 16, this change converts actions-riff-raff to a composite action. Using a composite action, we can run the action using Node 18. The main drawback to this approach is having to copy all of the action's inputs to the executing shell's environment.

Once actions/runner supports Node 20, we could revert this change and go back to using a Javascript Action.

Resolves #33.

What does this change?

  • Sets the node version to 18.17.0
  • Installs @types/node@18.17.0. The major.minor version of @types/node should match the major.minor version of node specified in .nvmrc (thx @mxdvl for showing me this!)
  • Converts the action to a composite action which executes dist/index.js using the node version defined in .nvmrc. This requires copying all the action inputs to the shell's environment

How to test

I haven't figured out a thorough way of validating this change, but anecdotally I have noticed that the S3 upload error doees not occur following this change 1.

Footnotes

  1. https://github.com/guardian/frontend/actions/runs/5666984810/job/15354810970?pr=26058

JavaScript actions have no way of supporting Node 18. We want to use
Node 18 to fix #33.

It seems that GitHub will never support `node18` for `actions/runner`,
instead jumping straight to Node 20.
@georgeblahblah georgeblahblah requested a review from a team July 26, 2023 10:24
Copy link
Contributor

@nicl nicl left a comment

Choose a reason for hiding this comment

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

Thanks for this @georgeblahblah ! The linked issue around their plans to migrate to v20 is helpful in motivating this change - as doesn't look like they will update very soon.

I've tested this branch on one of our projects too without issue. And, given there are no actual TS/code changes, think this is good.

Once merged, update the v2 release by something like the following:

git switch main
git pull origin HEAD
git tag v2
git push origin v2 -f

And then also manually create a new v2.2.1 release within the Github UI with a notes on the issue being fixed.

In practice, everyone seems to pin to just the major version (v2) but additional releases are helpful for those who want a bit more stability.

Copy link
Contributor

@mxdvl mxdvl 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 composite actions are cooler! 🧊

package.json Outdated Show resolved Hide resolved
The `major.minor` version of `@types/node` should match the version of
node specified in `.nvmrc`
@georgeblahblah georgeblahblah merged commit 9e85c59 into main Jul 26, 2023
1 check passed
@georgeblahblah georgeblahblah deleted the gb/node-18 branch July 26, 2023 14:03
@coldlink
Copy link
Member

coldlink commented Jul 26, 2023

Hey all, this is causing @guardian/actions-riff-raff to fail on projects without an .nvmrc file, such as Scala only projects, e.g.

https://github.com/guardian/identity/actions/runs/5670168291/attempts/1

@georgeblahblah
Copy link
Contributor Author

Hey all, this is causing @guardian/actions-riff-raff to fail on projects without an .nvmrc file, such as Scala only projects, e.g.

https://github.com/guardian/identity/actions/runs/5670168291/attempts/1

😱 oh no! #61 should fix that. You can specify guardian/actions-riff-raff@2.2.0 in the meantime if it's causing you trouble.

@akash1810
Copy link
Member

https://github.blog/changelog/2023-09-22-github-actions-transitioning-from-node-16-to-node-20/ looks like GHA support for Node 20 is now available.

akash1810 added a commit that referenced this pull request Oct 2, 2023
Manually update `dist/index.js` following various dependency updates.

Also updates esbuild configuration to target Node 18, following #60.
georgeblahblah added a commit that referenced this pull request Oct 12, 2023
We migrated to a composite action in
#60 in order to run
the action's code on Node 18 before it was supported by the action
runner.

GitHub have since released support for runners on Node 20.
georgeblahblah added a commit that referenced this pull request Oct 16, 2023
* Switch from a composite action to a Node 20 Javascript action

We migrated to a composite action in
#60 in order to run
the action's code on Node 18 before it was supported by the action
runner.

GitHub have since released support for runners on Node 20.
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.

Uploading to S3 can fail
5 participants