Skip to content

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

Merged
merged 2 commits into from
Aug 20, 2025

Conversation

lukekarrys
Copy link
Member

@lukekarrys lukekarrys commented Aug 20, 2025

Description

Update to npm@11 in the publish-packages workflow to ensure that OIDC is supported.

This could also pin to a specific npm version instead of using the entire v11 range.

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 publish and it worked to update npm in this way.

Related Issues

#8085

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run pnpm format to ensure the code follows the style guide.
  • I have run pnpm test to check if all tests are passing.
  • I have run pnpm build to check if the website builds without errors.
  • I've covered new added functionality with unit tests if necessary.

Signed-off-by: Luke Karrys <luke@lukekarrys.com>
@Copilot Copilot AI review requested due to automatic review settings August 20, 2025 18:30
@lukekarrys lukekarrys requested a review from a team as a code owner August 20, 2025 18:30
Copy link

vercel bot commented Aug 20, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Updated (UTC)
nodejs-org Ready Ready Preview Aug 20, 2025 7:03pm

Copy link
Contributor

@Copilot Copilot AI left a 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.

Copy link

codecov bot commented Aug 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.17%. Comparing base (25167af) to head (76b456f).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

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 feedback on the report? Share it here.

@MattIPv4
Copy link
Member

🤔 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).

@lukekarrys
Copy link
Member Author

lukekarrys commented Aug 20, 2025

Have we tested the OIDC publish as it is without this change?

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.

It seems odd to me that we need to install a specific version of npm in CI, when we're using pnpm, not npm

pnpm will use the npm available on the current machine as confirmed in this issue: pnpm/pnpm#9812 (comment). So I believe it is necessary to update to a specific version of npm to use OIDC.

this also seems very fragile to future npm updates

I agree with this which is why I mentioned maybe it's better to use install npm@11.5.1? That would make it more stable but still necessary to update manually I think.

@MattIPv4
Copy link
Member

Have we tested the OIDC publish as it is without this change?

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.

@avivkeller are you able to test the publish workflow with the OIDC change?

@MattIPv4
Copy link
Member

It seems odd to me that we need to install a specific version of npm in CI, when we're using pnpm, not npm

pnpm will use the npm available on the current machine as confirmed in this issue: pnpm/pnpm#9812 (comment). So I believe it is necessary to update to a specific version of npm to use OIDC.

this also seems very fragile to future npm updates

I agree with this which is why I mentioned maybe it's better to use install npm@11.5.1? That would make it more stable but still necessary to update manually I think.

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 >= 11.5.1? And then, in CI, we have some logic that installs that as a minimum if what is default does not match the constraint?

@avivkeller
Copy link
Member

@avivkeller are you able to test the publish workflow with the OIDC change?

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.

That would make it more stable but still necessary to update manually I think.

It wouldn't need to be there that long, only until Node.js v24 is LTS

@ovflowd
Copy link
Member

ovflowd commented Aug 20, 2025

Oh hi @lukekarrys, been a minute, how's going? <3

@MattIPv4
Copy link
Member

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.

I feel like as part of this PR, or in another, we should fix that the CI "passes" when it fails to publish 😂

@avivkeller
Copy link
Member

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.

@MattIPv4
Copy link
Member

👍 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 <me@aviv.sh>
@avivkeller avivkeller added fast-track Fast Tracking PRs github_actions:pull-request Trigger Pull Request Checks labels Aug 20, 2025
@github-actions github-actions bot removed the github_actions:pull-request Trigger Pull Request Checks label Aug 20, 2025
Copy link
Contributor

github-actions bot commented Aug 20, 2025

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🟢 98 🟢 100 🟢 100 🟢 100 🔗
/en/about 🟢 100 🟢 97 🟢 100 🟠 88 🔗
/en/about/previous-releases 🟢 99 🟢 93 🟢 100 🟠 89 🔗
/en/download 🟢 95 🟢 100 🟢 100 🟢 100 🔗
/en/blog 🟢 100 🟢 100 🟢 96 🟢 100 🔗

@avivkeller avivkeller mentioned this pull request Aug 20, 2025
4 tasks
@lukekarrys
Copy link
Member Author

@avivkeller Looks good! Now that the suggestions have been applied I just wanted to check that there was nothing else for me to do?

@avivkeller
Copy link
Member

That's it on my mind, once @nodejs/web-infra approves, this can be merged

@MattIPv4 MattIPv4 added this pull request to the merge queue Aug 20, 2025
Merged via the queue into main with commit 660f38f Aug 20, 2025
19 checks passed
@MattIPv4 MattIPv4 deleted the lukekarrys-patch-1 branch August 20, 2025 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track Fast Tracking PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants