drop node 12 / matrix against `current`, and latest 3 `LTS` node versions by theinterned · Pull Request #369 · github/eslint-plugin-github

theinterned · 2022-12-28T22:50:05Z

I noticed in this PR that tests are failing in node 12 but running successfully in node 14, 16, and 18.

Given that node 12 is End-of-Life while 18 is LTS, I think we should just drop testing again 12.

I also changed the matrix to dynamically test against: ~~latest~~ current† (v19), lts/* (v18 is current LTS), lts/-1 (v16), and lts/-2 (v14). See node release schedule. This way these will update as node versions roll along.

There was concern that the dynamic nature would cause confusion when node versions rolled over. To counter-act this, @smockle had the great idea to create an issue when tests start failing on current. So I've set that up:

When tests fail on current, we continue-on-error (meaning the job stays green). Instead we create an issue. Here's an example of that: #378

† Note: I was originally using latest but switched to current. These are aliases, but I switched to current as that's the word used in the node release schedule.

.github/workflows/nodejs.yml

Co-authored-by: Nick Schonning <nschonni@gmail.com>

srt32 · 2023-01-03T15:49:06Z

.github/workflows/nodejs.yml

    strategy:
      matrix:
-        node-version: [12.x, 14.x, 16.x]
+        node-version:


I wonder if we'll get confused / have some unexpected behavior when these things turn over.

Like, the day that a new version gets added, what if CI starts failing? That would be annoying to fix without a hardcoded list. WDYT?

unexpected behavior when these things turn over

Because the matrix includes latest, when lts/* begins pointing to a new version, no failures should occur, because that new version would have already been tested, back when it was latest. So floating version (viz. lts/-2, lts/-1, lts/*) values ought to be safe.

That said, when latest itself begins pointing to a new version, failures might occur. But if we remove latest, then lts/* loses the safety described above.

when latest itself begins pointing to a new version, failures might occur

Here’s one workaround: For latest (and only latest), if any step of the job fails (failure()), open an issue, then exit 0, so the overall workflow run is still green.

So, we’d still be notified of failures, but they wouldn’t block PRs (unless another version is also failing), etc.

Okay! cool. I got this working!

Here's a sample failed run: https://github.com/github/eslint-plugin-github/actions/runs/3884555154, and the issue it created: #378

To test this I created a failing test 20e83a5 which I will now delete.

nschonni · 2023-01-10T14:50:28Z

You seem to be going to a lot of effort just to avoid using the version numbers in the matrix. Those changes happen infrequentely and are also a good time to evaluated if major versions of things like ESLint need to change. They usually have a major bump when version support changes

nschonni · 2023-01-10T15:25:14Z

I'd suggest taking a look at what some of the community supported extensions do like https://github.com/eslint-community/eslint-plugin-promise/blob/main/.github/workflows/CI.yml
The CI matches the supported versions of ESLint, including some older ones that wont be dropped till a semver major release of ESLint. They also have cross version testing of the last major version of ESLint.
Since this is an internal plug-in, you've got more flexibility on choosing what you want to support though.

theinterned · 2023-01-10T15:26:57Z

You seem to be going to a lot of effort just to avoid using the version numbers in the matrix

Yeah maybe but it was a fun to work on little challenge and I learned some stuff 🤷

Those changes happen infrequentely and are also a good time to evaluated if major versions of things like ESLint need to change. They usually have a major bump when version support changes

Note that current doesn't fail CI — but this kind of signal could alert us that there's work to do to support the next version.

nschonni · 2023-01-10T15:30:11Z

Note that current doesn't fail CI — but this kind of signal could alert us that there's work to do to support the next version.

Yeah, but that support likely needs to come from ESLint first. Their matrix is also explicit, and is used to test their supported versions https://github.com/eslint/eslint/blob/2952d6ed95811ce0971b6855d66fb7a9767a7b72/package.json#L168-L170

nschonni · 2023-01-10T15:31:34Z

Anyway, I'm going to unsubscribe from this as I think we're bikeshedding at this point, and it's not worth either of our time

jfuchs

Question not blocking.

jfuchs · 2023-01-10T16:42:02Z

.github/workflows/nodejs.yml

+        id: test
        run: npm test
+        continue-on-error: ${{ matrix.node-version == 'current' }}
+      - name: Create Issue on Fail


Is this going to create a new issue on every failure?

Yes it will. Can you think of a way to improve that?

theinterned · 2023-01-10T17:17:08Z

Maybe this is all too much. Dropping this dynamic approach in favour of #380

It was still a fun little project.

drop node 12 / add node 18 to test matrix

9a3b0cd

theinterned requested a review from a team as a code owner December 28, 2022 22:50

theinterned mentioned this pull request Dec 28, 2022

chore(deps-dev): bump eslint-plugin-eslint-plugin from 5.0.3 to 5.0.7 #368

Merged

theinterned self-assigned this Dec 28, 2022

nschonni reviewed Dec 28, 2022

View reviewed changes

.github/workflows/nodejs.yml Outdated Show resolved Hide resolved

theinterned and others added 2 commits December 29, 2022 12:06

also matrix for latest node (19)

5057fbc

Co-authored-by: Nick Schonning <nschonni@gmail.com>

use dynamic LTS and latest versions

a8fa032

theinterned changed the title ~~drop node 12 / add node 18 to test matrix~~ drop node 12 / matrix against latest, and latest 3 LTS node versions Dec 29, 2022

theinterned mentioned this pull request Dec 29, 2022

feat: role-supports-aria-props rule (no aria-label-misuse) #362

Merged

srt32 reviewed Jan 3, 2023

View reviewed changes

theinterned force-pushed the theinterned/drop-node-12-from-test-matrix branch from ad3520f to 22e58b4 Compare January 9, 2023 23:02

theinterned changed the title ~~drop node 12 / matrix against latest, and latest 3 LTS node versions~~ drop node 12 / matrix against current, and latest 3 LTS node versions Jan 10, 2023

theinterned requested review from jfuchs, nschonni, smockle and srt32 and removed request for nschonni January 10, 2023 15:17

theinterned added 4 commits January 10, 2023 10:20

add test to fail on latest node version

20e83a5

fail on latest node version

f273fd9

create an issue on current node fail

786bc35

remove failing node-version test

4f9be41

theinterned force-pushed the theinterned/drop-node-12-from-test-matrix branch from 8b73ee5 to 4f9be41 Compare January 10, 2023 15:21

This was referenced Jan 10, 2023

Test failed on Node.js current (v19.4.0) #379

Closed

Test failed on Node.js current (v19.4.0) #378

Closed

jfuchs approved these changes Jan 10, 2023

View reviewed changes

theinterned mentioned this pull request Jan 10, 2023

drop node 12 / add node 18 to test matrix #380

Merged

theinterned closed this Jan 10, 2023

Conversation

theinterned commented Dec 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

srt32 Jan 3, 2023

Choose a reason for hiding this comment

Uh oh!

smockle Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smockle Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theinterned Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nschonni commented Jan 10, 2023

Uh oh!

nschonni commented Jan 10, 2023

Uh oh!

theinterned commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nschonni commented Jan 10, 2023

Uh oh!

nschonni commented Jan 10, 2023

Uh oh!

jfuchs left a comment

Choose a reason for hiding this comment

Uh oh!

jfuchs Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

theinterned Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

theinterned commented Jan 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

theinterned commented Dec 28, 2022 •

edited

Loading

smockle Jan 4, 2023 •

edited

Loading

smockle Jan 4, 2023 •

edited

Loading

theinterned Jan 10, 2023 •

edited

Loading

theinterned commented Jan 10, 2023 •

edited

Loading