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 github workloads #478

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

camillobruni
Copy link
Contributor

@camillobruni camillobruni commented Jan 29, 2025

  • Use latest action versions
  • Bump node version to current stable v22
  • Use caches for node modules to slightly speed up setup
  • Mild formatting was applied

Copy link

netlify bot commented Jan 29, 2025

Deploy Preview for webkit-speedometer ready!

Name Link
🔨 Latest commit a8686c5
🔍 Latest deploy log https://app.netlify.com/sites/webkit-speedometer/deploys/67a2326a79fad3000894db8f
😎 Deploy Preview https://deploy-preview-478--webkit-speedometer.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@rniwa rniwa left a comment

Choose a reason for hiding this comment

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

Looks okay to me.

.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
@camillobruni
Copy link
Contributor Author

Simplified the test.yml file based on the feedback.
Looks like the netlify deployment is now unhappy with the upped node version requirement. @julienw is this easy to fix or should I just lower the requirements again?

@julienw
Copy link
Contributor

julienw commented Feb 4, 2025

Simplified the test.yml file based on the feedback. Looks like the netlify deployment is now unhappy with the upped node version requirement. @julienw is this easy to fix or should I just lower the requirements again?

image
just changed it :)

Another way would be to use a .node-version or .nvmrc file in the repository (this would also override any setting in the configuration UI BTW). I think node-version-file can also use that one BTW.
It's a bit too bad that netlify can't use the version specified in package.json :(

Copy link
Contributor

@julienw julienw left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me!

@@ -3,7 +3,7 @@
"version": "3.0.0-alpha",
"description": "An open source repository for the Speedometer benchmark.",
"engines": {
"node": ">=18.13.0",
"node": ">=22.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to ensure that we stay on v22 by adding < 23 as well? I'm a bit afraid that this will switch to v24 automatically and we'll get build errors. We can always change that later if this happens of course.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you need to run npm i to update package-lock.json as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yeah... of course.
Let me add a CI check for that as well in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think npm ci does it for dependencies, it's disappointing it doesn't work for everything that can update it!

cache: "npm"

- name: Install Node Packages
run: npm ci
Copy link
Contributor

Choose a reason for hiding this comment

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

much better!

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