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

chore: add check engines script to CI #2922

Merged
merged 1 commit into from
Oct 28, 2023
Merged

chore: add check engines script to CI #2922

merged 1 commit into from
Oct 28, 2023

Conversation

lukekarrys
Copy link
Member

@lukekarrys lukekarrys commented Oct 27, 2023

This will test against regressions in (sub-)dependency engine changes and guard against breaking changes like #2848 #2873.

Errors will look like this:

Error: The following production dependencies are not compatible with `engines.node: ^14.17.0 || ^16.13.0 || >=18.0.0` found in `/home/runner/work/node-gyp/node-gyp/package.json`:
[email protected]:
  engines.node: ^16.14.0 || >=18.0.0
  location: node_modules/make-fetch-happen
[email protected]:
  engines.node: ^16.13.0 || >=18.0.0
  location: node_modules/which

@lukekarrys lukekarrys changed the title chore: add check engines script to CI Add engines check and update dependencies/engines Oct 27, 2023
@lukekarrys lukekarrys added this to the v10.0.0 milestone Oct 27, 2023
@lukekarrys lukekarrys force-pushed the lk/check-engines branch 3 times, most recently from ddb6020 to 7cba0db Compare October 28, 2023 00:33
Copy link
Contributor

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

Adding this check seems like a good idea! (Given how it impacted users last time dependencies were bumped outside of the "advertised" supported versions of Node.)

I haven't read through all the code or anything, but I would approve of doing this, generally speaking!

I tried moving it to its own workflow file on my fork, seems to work just fine: DeeDeeG@7564e32

Feel free to copy that if wanting it in its own separate GitHub Actions workflow file (separate .yml CI file).

@lukekarrys
Copy link
Member Author

@DeeDeeG The comment I left in the code was too vague, sorry about that. I meant I would like to move the whole thing to its own GitHub action to be used like uses: pkgjs/check-engines.

I use this same action other places and I'd love to find a good org for to be used ecosystem-wide. I talked a bit to some folks at the pkgjs org and I think that might be a good place for it.

@lukekarrys lukekarrys changed the title Add engines check and update dependencies/engines chore: add check engines script to CI Oct 28, 2023
@lukekarrys lukekarrys merged commit 21a7249 into main Oct 28, 2023
31 checks passed
@lukekarrys lukekarrys deleted the lk/check-engines branch October 28, 2023 03:51
@DeeDeeG
Copy link
Contributor

DeeDeeG commented Oct 28, 2023

You might have seen this already, but the updated engines: field got removed in one of the force pushes, and some just-merged dependency bumps are failing the "Check Engines" job in CI without that updated engines: value.

NodeJS 14 is still in there, and the 16 is still ^16.13.0 instead of ^16.14.0.

EDIT: Okay, yep, fixed just now via #2929. Disregard this comment.

@lukekarrys
Copy link
Member Author

I was having some trouble getting the tests passing in CI so I decided to separate out all the commits into separate PRs. It did end up showing that the check-engines job will work though! 😄

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.

2 participants