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

Fix CI job #203

Merged
merged 1 commit into from May 8, 2023
Merged

Fix CI job #203

merged 1 commit into from May 8, 2023

Conversation

Magellol
Copy link
Member

@Magellol Magellol commented May 8, 2023

Overview

The size-limit job was failing due to github updating the node version installed by default on their runner to be 18 since it became LTS. Ref: actions/runner-images#7002

There are a couple of ways to solve this:

  • Pin the node version in the size workflow to be the same as the one in main.
  • Possibly upgrade the size-limit package
  • Merge workflows so everything uses the same pinned versions

The last one is the one I chose to do now to unblock #202 because we haven't tested anything regarding node 18 yet. Also I don't think we need two workflows.

@Magellol Magellol marked this pull request as ready for review May 8, 2023 14:28
@Magellol Magellol requested a review from a team as a code owner May 8, 2023 14:28
@Magellol Magellol requested a review from astlouisf May 8, 2023 14:28
@Magellol Magellol mentioned this pull request May 8, 2023
@github-actions
Copy link

github-actions bot commented May 8, 2023

size-limit report 📦

Path Size
dist/unsplash-js.cjs.production.min.js 4.21 KB (0%)
dist/unsplash-js.esm.js 4.22 KB (0%)

@astlouisf
Copy link

astlouisf commented May 8, 2023

Isn't a side effect of merging both workflows that size-limit gets run for each platform of the matrix? Is it necessary? If not, it would be better to avoid unnecessary work.

@Magellol
Copy link
Member Author

Magellol commented May 8, 2023

@astlouisf We can condition around and make it run on a single node version if we wish to do so. Alternatively, we can keep the separate workflow but add the setup node step to pin a version but which would would we pin? 16?

@astlouisf
Copy link

16 isn't even in the current matrix; 10 and 12 are past 'end-of-life' 🤣. There is some maintenance to do here.

In any case, my initial comment was only meant to highlight the duplicated work introduced. Considering how active this package is, I don't think this duplication is that much of an concern (doesn't affect dx that much and won't blow through our budget). I'll let you judge if you want to tackle it or not, considering it is blocking your other PR.

@Magellol Magellol merged commit c5e4ba2 into master May 8, 2023
9 checks passed
@Magellol Magellol deleted the fix-ci-jobs branch May 8, 2023 16:57
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.

None yet

2 participants