-
-
Notifications
You must be signed in to change notification settings - Fork 55
ci: enable trusted publishing #419
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
https://docs.npmjs.com/trusted-publishers Signed-off-by: JounQin <[email protected]>
|
WalkthroughUpdated GitHub Actions workflows: added an "Upgrade npm" step in the release workflow, removed an env var from Node.js setup and removed NPM provenance/token settings from the release publish step; adjusted the CI matrix by replacing Node.js 24 with 24.5 and adding a comment referencing Node.js issue #59480. Changes
Sequence Diagram(s)sequenceDiagram
participant Repo as Repository
participant Runner as GitHub Actions Runner
participant Registry as npm Registry
Repo->>Runner: Trigger release workflow
Runner->>Runner: Setup Node.js (LTS)
Runner->>Runner: Upgrade npm (npm install -g npm@latest)
Runner->>Runner: Build / prepare release
Runner->>Registry: Publish package (no NPM_TOKEN configured)
Registry-->>Runner: Publish response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
commit: |
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.
Important
Looks good to me! 👍
Reviewed everything up to 42b48c9 in 1 minute and 16 seconds. Click for details.
- Reviewed
24lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/release.yml:29
- Draft comment:
Removal of SKIP_YARN_COREPACK_CHECK:1 was done without explanation. Confirm that upgrading npm now obviates that workaround and add a comment referencing the GitHub issue for context. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. .github/workflows/release.yml:53
- Draft comment:
Setting NPM_TOKEN to an empty string disables npm auth. Verify this is intentional for trusted publishing and add a clarifying comment so future maintainers understand the rationale. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment asks for verification and explanation, which violates our rules. It's asking the author to confirm their intention and add documentation. While the suggestion might be well-intentioned, we should trust that the author knows what they're doing with npm authentication in their release workflow. Maybe understanding npm authentication is critical for security, and having clear documentation about intentionally disabled auth could prevent future security issues? While documentation is good, our rules explicitly state not to ask authors to verify their intentions or add explanatory comments. The change is likely intentional given it's in a release workflow. Delete the comment as it violates our rules by asking for verification and additional documentation.
Workflow ID: wflow_YUfIdBbm5ue17OKj
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #419 +/- ##
=======================================
Coverage 96.10% 96.10%
=======================================
Files 96 96
Lines 4952 4952
Branches 1862 1837 -25
=======================================
Hits 4759 4759
Misses 192 192
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
33-35: Pin npm to a minimum trusted-publishing-capable version instead oflatest.Upgrading to
npm@latestcan introduce volatility. For npm Trusted Publishers, you only need a recent 10.x. Pin to a safe minimum to keep builds reproducible.Apply:
- - name: Upgrade npm - run: npm install -g npm@latest + - name: Upgrade npm + run: npm install -g npm@^10.8.0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/release.yml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on macos-latest
🔇 Additional comments (1)
.github/workflows/release.yml (1)
52-53: Confirmed:changeset publishuses npm CLI
We’ve verified that yourreleasescript ("clean-pkg-json && changeset publish") does not invokeyarn npm publishor any Yarn-based publish plugin. By default, Changesets calls the npm CLI under the hood, so removingNPM_TOKENand relying on OIDC is correct.Optional: if you’d like package provenance, add to your package.json:
{ "publishConfig": { "provenance": true } }
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.
Important
Looks good to me! 👍
Reviewed 6e3eb34 in 57 seconds. Click for details.
- Reviewed
14lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/ci.yml:25
- Draft comment:
Replaced Node 24 with 24.5 per nodejs/node#59480. Confirm this minor version meets your CI requirements. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_kueOZ8qjVgI0zSOG
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
24-25: Good pin; quote or use 24.5.x to avoid YAML coercion and ease future patch uptakePinning to Node 24.5 with a link to the upstream issue is sensible. Two small nits:
- Quote the version (or use 24.5.x) so YAML doesn’t treat it as a float and to make intent clear.
- Optional: make the comment explicit that this is temporary and should be reverted to 24 when the issue is resolved.
Apply this diff:
- # https://github.com/nodejs/node/issues/59480 - - 24.5 + # Temporary pin due to https://github.com/nodejs/node/issues/59480 — revert to '24' once resolved + - "24.5.x"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/ci.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 24.5 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24.5 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 24.5 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on macos-latest
Signed-off-by: JounQin <[email protected]>
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.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
.github/workflows/release.yml (1)
44-53: Enable Provenance for Trusted PublishingYour release path (
yarn release→clean-pkg-json && changeset publish) doesn’t pass the--provenanceflag or setNPM_CONFIG_PROVENANCE, so npm Trusted Publishing under OIDC may fail. Please update your workflow to include provenance:
- File: .github/workflows/release.yml (lines 44–53)
- name: Create Release Pull Request or Publish to npm id: changesets uses: changesets/action@v1 with: commit: 'chore: release eslint-plugin-import-x' title: 'chore: release eslint-plugin-import-x' publish: yarn release env:GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}NPM_CONFIG_PROVENANCE: trueAlternatively, adjust your
releasescript in package.json to invoke npm publish with--provenance. This guarantees that provenance metadata is recorded during the publish step.
♻️ Duplicate comments (1)
.github/workflows/release.yml (1)
44-53: Removing NPM_TOKEN aligns with Trusted Publishers; no need to set it to an empty string.Agree with dropping
NPM_TOKENentirely to avoid_authToken=undefinedchurn in Changesets. This matches prior feedback.
🧹 Nitpick comments (2)
.github/workflows/release.yml (2)
33-35: Make npm upgrade deterministic (pin version or use Corepack) instead of floatinglatest.Using
npm@latestcan introduce nondeterministic CI behavior when npm releases new versions. Prefer pinning to a supported major/minor that satisfies Trusted Publishers, or activate it via Corepack for this job scope.Apply one of the following diffs:
Option A — pin a major for stability:
- - name: Upgrade npm - run: npm install -g npm@latest + - name: Upgrade npm + run: npm install -g npm@10Option B — activate with Corepack (scoped to the job, avoids global mutation):
- - name: Upgrade npm - run: npm install -g npm@latest + - name: Upgrade npm + run: | + corepack enable + corepack prepare npm@10 --activate
46-46: Pin changesets/action to a commit SHA for supply-chain hardening.Other actions are pinned;
changesets/action@v1isn’t. Pin to a known-good commit SHA to match your pinning policy.Example (replace HASH with the desired commit SHA):
- uses: changesets/action@v1 + uses: changesets/action@<PINNED_COMMIT_SHA>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/release.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on macos-latest
🔇 Additional comments (1)
.github/workflows/release.yml (1)
12-16: Good: OIDC permission is correctly configured for npm Trusted Publishing
id-token: writeis present at the workflow level. This is required for npm Trusted Publishers. No action needed.
https://docs.npmjs.com/trusted-publishers
Important
Enable trusted publishing by upgrading npm and streamlining configurations in CI/CD workflows.
release.ymlfor trusted publishing.release.yml.release.yml.24.5inci.ymldue to issue #59480.This description was created by
for 6e3eb34. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Chores
Notes