-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
chore: ensure npm@11 is installed for OIDC publishing #8091
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
Conversation
Signed-off-by: Luke Karrys <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Pull Request Overview
Updates the publish-packages workflow to install npm@11 globally to ensure OIDC (OpenID Connect) support for package publishing.
- Adds a setup step to install npm@11 globally before publishing packages
- Ensures compatibility with OIDC authentication which requires npm >=11.5.1
- Maintains existing workflow structure while adding necessary npm version dependency
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8091 +/- ##
==========================================
+ Coverage 73.11% 73.17% +0.05%
==========================================
Files 97 97
Lines 8440 8440
Branches 229 228 -1
==========================================
+ Hits 6171 6176 +5
+ Misses 2268 2263 -5
Partials 1 1 ☔ View full report in Codecov by Sentry. |
|
🤔 Have we tested the OIDC publish as it is without this change? It seems odd to me that we need to install a specific version of npm in CI, when we're using pnpm, not npm (this also seems very fragile to future npm updates as this is another place we'd need to remember to update). |
I'm not super familiar with this repo but it looks like the last publish was 5 days ago and the OIDC PR (#8085) was merged yesterday. It might be better to hold off on this PR until the next publish.
I agree with this which is why I mentioned maybe it's better to use install |
@avivkeller are you able to test the publish workflow with the OIDC change? |
Ah okay, makes sense. I wonder if we can put this in our package.json instead, and just leave it as an open requirement for |
https://github.com/nodejs/nodejs.org/actions/runs/17107462287/job/48520018615#step:7:21 only installed npm 10, so, no, it does not publish. I tested it locally before making that PR, but I have npm 11+, so I didn't catch this.
It wouldn't need to be there that long, only until Node.js v24 is LTS |
|
Oh hi @lukekarrys, been a minute, how's going? <3 |
I feel like as part of this PR, or in another, we should fix that the CI "passes" when it fails to publish 😂 |
I'll make that in a follow-up. @lukekarrys After my comments are addressed, i'm fine to fast track this, seeing as in a month or two v24 will be LTS. |
|
👍 Hadn't realised this'll be default in v24 -- yeh this seems fine to hard-code for now then, we should open an issue immediately to remind us to remove this. |
Signed-off-by: Aviv Keller <[email protected]>
|
Lighthouse Results
|
|
@avivkeller Looks good! Now that the suggestions have been applied I just wanted to check that there was nothing else for me to do? |
|
That's it on my mind, once @nodejs/web-infra approves, this can be merged |
…blishing per nodejs/nodejs.org#8091 we need to do a little extra work to make sure npm is new enough, since pnpm uses it behind the scenes to publish to the npm repository also tweak the README to get a test release of core
…blishing per nodejs/nodejs.org#8091 we need to do a little extra work to make sure npm is new enough, since pnpm uses it behind the scenes to publish to the npm repository also tweak the README to get a test release of core
…blishing (#399) per nodejs/nodejs.org#8091 we need to do a little extra work to make sure npm is new enough, since pnpm uses it behind the scenes to publish to the npm repository also tweak the README to get a test release of core
Description
Update to
npm@11in thepublish-packagesworkflow to ensure that OIDC is supported.This could also pin to a specific npm version instead of using the entire
v11range.Validation
I can't confirm that is working without running the workflow but I did see that a similar change to #8085 failed in the node-gyp repo (nodejs/node-gyp#3201). npm's docs say that npm >=11.5.1 is required to use OIDC. I've also run a similar workflow in a work repo that used
pnpm publishand it worked to update npm in this way.Related Issues
#8085
Check List
pnpm formatto ensure the code follows the style guide.pnpm testto check if all tests are passing.pnpm buildto check if the website builds without errors.