-
Notifications
You must be signed in to change notification settings - Fork 990
Preparing for NPM trusted publishing plus node version housekeeping #2952
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
base: master
Are you sure you want to change the base?
Conversation
publishing. Updating node versions to reflect support matrix.
| - "v*.*.*" | ||
|
|
||
| permissions: | ||
| id-token: write |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were you able to configure a trusted publisher on npmjs? Otherwise I can dig into doing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't set up that side of it yet, so be my guest!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof, I don't see a way to do it for all of the packages at once. I'll probably click through all of them one at a time later today 😩.
I'm going to configure it like this. Note that I added a permissive release environment in GitHub now, which we can configure later without having to go to all 1xx packages and reconfigure it later.
After that, per the docs guidance, I will then also set this:
| strategy: | ||
| matrix: | ||
| node-version: [18.x, 20.x, 22.x] | ||
| node-version: [20.x, 22.x, 24.x] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen other packages consider dropping support for old versions of nodejs as a breaking change.
We aren't technically breaking support here, just not testing it which would make it more likely for a breaking change to sneak in.
I'm happy to either merge this as a non-major change, or push it off for later if you'd prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to tread cautiously. So add 18 back in and keep 24 too? Or cap it at 3 - 18, 20, 22?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think we can just add 24. Once this merges we need to make 24 required.
|
Putting on hold until I can confirm pnpm support for trusted publishers ... |
NPM is recommending avoiding using NPM tokens for publishing, instead favouring trusted publishers (e.g. a particular github workflow). This change: