Codecov bot is not sending a coverage report to this PR because it can't find previous report frommain (I assume this is because the last report was uploaded in september):

https://app.codecov.io/gh/typescript-eslint/typescript-eslint/pull/8167

_{Ohh, 729 commits on main without coverage reports}

chore: unpin primary node version in ci

b81b483

Copy link

Contributor

typescript-eslintbot commentedJan 2, 2024

Thanks for the PR,@auvred!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

Copy link

netlifybot commentedJan 2, 2024•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	`0812265`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/659b8d017444f40008faba92
😎 Deploy Preview	https://deploy-preview-8167--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 90 (🔴 down 9 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

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

Merge branch 'main' into chore/8166

8e8cbaa

auvred marked this pull request as ready for review

January 5, 2024 12:16

auvred mentioned this pull request

Jan 5, 2024

Repo: codecov coverage reports are not longer uploaded#8166

Closed

Merge branch 'main' into chore/8166

0812265

JoshuaKGoldberg requested changes

Jan 11, 2024

View reviewed changes

Copy link

Member

JoshuaKGoldberg 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 hopping onto this! I think I'd rather a more pin-oriented fix direction if we can make it?

.github/workflows/ci.yml

		@@ -14,7 +14,7 @@ concurrency:
		cancel-in-progress: true

		env:
		PRIMARY_NODE_VERSION:'>=20.6.1'
		PRIMARY_NODE_VERSION:20

Copy link

Member

JoshuaKGoldbergJan 11, 2024

Choose a reason for hiding this comment

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

Hmm, can we go the other way? Changingnode-version to be 16 and the pinned primary node version?

According tohttps://github.com/orgs/community/discussions/26388 andhttps://stackoverflow.com/questions/74072206/github-actions-use-variables-in-matrix-definition it's not a straightforward [16, ${{ env.PRIMARY_NODE_VERSION }}] 🙃 ...[16, "$PRIMARY_NODE_VERSION"] might not work either then. I haven't tried it.

So maybe astartsWith kind of check is necessary?

I'd rather not unpin a version and introduce unexpected version changes if we can avoid it.

Copy link

MemberAuthor

auvredJan 13, 2024•
edited
Loading

Choose a reason for hiding this comment

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

Yes, it looks like we can't useenv context in matrix definition :(

You can use the env context in any key in a workflow step except for the id and uses keys.

^ Ref:GitHub docs

I'd rather not unpin a version and introduce unexpected version changes if we can avoid it.

Onmain branch (with pinnedPRIMARY_NODE_VERSION: '>=20.6.1')actions/setup-node@v3 action downloadsv20.10.0

Run actions/setup-node@v3  with:    node-version: >=20.6.1    always-auth: false    check-latest: false    token: ***  env:    PRIMARY_NODE_VERSION: >=20.6.1    NX_CLOUD_ACCESS_TOKEN: ***Found in cache @ /opt/hostedtoolcache/node/20.10.0/x64Environment details  node: v20.10.0  npm: 10.2.3  yarn: 3.7.0

So I think it's safe to unpin it!

It was pinned in this commit2a03e44 only two days later whenv20.6.1 was releasedhttps://github.com/nodejs/node/releases/tag/v20.6.1. I guess it was the only way to force it to be higher thanv20.6.0 (For some reason website tests were failing without Node.js being pinned (workflow runs))

But now the latestv20.x.x version of Node.js is used, whetherPRIMARY_NODE_VERSION is pinned or not

If we decide to leave it pinned and not touch it (I'd rather change it 😄), then we should use something scary like

startsWith(env.PRIMARY_NODE_VERSION, format('>={0}', matrix.node-version))

So, what do you think? Leaving it pinned makes sense to me as well, so if you decide to leave it pinned, I'll change all comparison expressions tostartsWith

Copy link

Member

JoshuaKGoldbergJan 30, 2024

Choose a reason for hiding this comment

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

Yeah that makes sense. My hesitation around unpinning is that I don't like "surprise" Node version bumps... ideally we'd have an exact version that gets updated over time the way Renovate bumps our versions of dependencies.

...but since we're already not doing that I think this is a straightforward bug fix. 😂