Skip to content
This repository has been archived by the owner on May 22, 2024. It is now read-only.

chore: run CI on latest Node.js #472

Merged
merged 2 commits into from
May 19, 2021
Merged

chore: run CI on latest Node.js #472

merged 2 commits into from
May 19, 2021

Conversation

ehmicky
Copy link
Contributor

@ehmicky ehmicky commented May 18, 2021

Part of https://github.com/netlify/team-dev/issues/19

This runs the CI on the latest Node.js version.

@ehmicky ehmicky added the type: chore work needed to keep the product and development running smoothly label May 18, 2021
@ehmicky ehmicky requested a review from eduardoboucas May 18, 2021 16:44
@ehmicky ehmicky self-assigned this May 18, 2021
@eduardoboucas
Copy link
Member

We should probably update https://github.com/netlify/zip-it-and-ship-it/blob/main/.github/workflows/benchmark.yml#L19 too. There's also https://github.com/netlify/zip-it-and-ship-it/blob/main/.github/workflows/release-please.yml#L25, but I wonder if we want to update that one too?

@ehmicky ehmicky force-pushed the chore/ci-node-latest branch 2 times, most recently from 6fb9b6c to 6705db4 Compare May 18, 2021 17:25
@ehmicky
Copy link
Contributor Author

ehmicky commented May 18, 2021

Good point, I missed those. Fixed!
Changing all of them would make sense with the goal of not having to update them in the future.

@github-actions
Copy link
Contributor

github-actions bot commented May 18, 2021

⏱ Benchmark results

Comparing with f188133

  • largeDepsEsbuild: 10.3s ⬇️ (14.12% decrease)
  • largeDepsZisi: 53.6s ⬇️ (15.52% decrease)

@ehmicky ehmicky force-pushed the chore/ci-node-latest branch 6 times, most recently from f02cb73 to c49231a Compare May 18, 2021 19:01
@ehmicky ehmicky force-pushed the chore/ci-node-latest branch from c49231a to fd59103 Compare May 18, 2021 19:03
@ehmicky ehmicky requested review from erezrokah and JGAntunes May 18, 2021 19:05
@@ -27,11 +27,12 @@ jobs:
uses: actions/setup-node@v2
with:
node-version: ${{ matrix.node-version }}
check-latest: true
Copy link
Member

Choose a reason for hiding this comment

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

What happens if we don't use check-latest here? From https://github.com/actions/setup-node:

When set to false, the action tries to first resolve a version of node from the local cache. (...) The local version of Node in cache gets updated every couple of weeks. If unable to find a specific version in the cache, the action will then attempt to download a version of Node.

And:

Setting check-latest to true has performance implications as downloading versions of Node is slower than using cached versions

I wonder about these performance implications as we try to speed up the CI runs. If we use * without check-latest, does it mean we may only get an updated version after GitHub updates the image? If so, how do we feel about this trade-off?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like download time is a few seconds
Linux is 4s, MacOS is 5s and Windows is 9s (based on this run).
This is a single sample, but I don't expect it to be much higher.

I've also subscribed to actions/setup-node#26 so we might be able to use lts in the future

Copy link
Member

Choose a reason for hiding this comment

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

Cool, in case that this looks good to me! 🚀

Copy link
Contributor Author

@ehmicky ehmicky May 19, 2021

Choose a reason for hiding this comment

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

I added check-latest: true because without it, * is tried against the cached Node.js versions which seemed to be 10, 12 and 14. So without it, * was somehow resolving to 14.*.*. :-/

I have experimented with several other ranges, and none worked: latest, current, *.*.*. I dug into the code of actions/setup-node doing the logic and it seems like this is mostly based on running semver.satisfies(), i.e. it expects semver ranges, not tags. The only one which did work was to use something like >1 but I was not 100% sure whether this would always behave correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: there is also this ongoing PR which is actually adding a library of mine to actions/setup-node and would allow using latest without needing check-latest: true.

@ehmicky ehmicky merged commit 9d5f1b8 into main May 19, 2021
@ehmicky ehmicky deleted the chore/ci-node-latest branch May 19, 2021 13:45
Skn0tt pushed a commit to netlify/build that referenced this pull request May 21, 2024
Co-authored-by: Eduardo Bouças <mail@eduardoboucas.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: chore work needed to keep the product and development running smoothly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants