-
Notifications
You must be signed in to change notification settings - Fork 3
ci: use-trusted-publishers #418
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
|
WalkthroughIntroduce a callable publish workflow with inputs and a reusable trusted-publish job; remove inline npm auth/env and per-registry publish steps; simplify snapshot workflow to call the publish workflow (passing beta prerelease/tag/access) and reduce workflow permissions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Developer
participant SnapshotWF as snapshot.yml
participant PublishWF as publish.yml (workflow_call)
participant TrustedJob as trusted-publish
participant Registry as npm Registry
Developer->>SnapshotWF: trigger (workflow_dispatch / push)
SnapshotWF->>PublishWF: workflow_call(inputs: branch, dist_tag=beta, prerelease=beta, access=public)
PublishWF->>TrustedJob: invoke trusted-publish job
TrustedJob->>TrustedJob: checkout -> setup pnpm/node -> install -> nx start-ci-run -> build/test/e2e
TrustedJob->>Registry: pnpm publish (dist-tag/access)
Registry-->>TrustedJob: publish result
TrustedJob-->>PublishWF: job result
PublishWF-->>SnapshotWF: finished
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Comment |
View your CI Pipeline Execution ↗ for commit 105551a
☁️ Nx Cloud last updated this comment at |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #418 +/- ##
=======================================
Coverage 55.63% 55.63%
=======================================
Files 32 32
Lines 2051 2051
Branches 344 344
=======================================
Hits 1141 1141
Misses 910 910 🚀 New features to boost your workflow:
|
Deployed 65ee3f2 to https://ForgeRock.github.io/ping-javascript-sdk/pr-418/65ee3f2cb3df2b6a72c82b370b3f56d41d71510d branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis➖ No Changes➖ @forgerock/davinci-client - 34.2 KB 11 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
3624e4d
to
732afa8
Compare
branches: | ||
- main | ||
# allow other workflows to call this one (for snapshots) | ||
workflow_call: |
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.
this is to allow snapshot.yml to call this workflow file.
jobs: | ||
# --- your existing release job: leave unchanged --- | ||
publish-or-pr: | ||
if: github.event_name == 'push' |
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.
make sure this is only on push.
732afa8
to
94c3219
Compare
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: 2
🧹 Nitpick comments (2)
.github/workflows/publish.yml (2)
164-168
: Tighten permissions for the trusted-publish job.Minimize to what’s needed for npm OIDC and artifact upload; drop issues/pull-requests unless you use them here.
Apply this diff:
permissions: - contents: write # read+write repo (okay for artifacts/logs) + contents: read id-token: write # REQUIRED: OIDC for npm Trusted Publishers - issues: write - pull-requests: write
215-219
: GH_TOKEN is unnecessary for local snapshot versioning.This step doesn’t push or create PRs; you can remove the token.
- - name: Version Packages as prerelease - run: pnpm changeset version --snapshot ${{ inputs.prerelease }} - env: - GITHUB_TOKEN: ${{ secrets.GH_TOKEN }} + - name: Version Packages as prerelease + run: pnpm changeset version --snapshot ${{ inputs.prerelease }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/publish.yml
(2 hunks).github/workflows/snapshot.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). (1)
- GitHub Check: pr
🔇 Additional comments (4)
.github/workflows/publish.yml (3)
6-8
: LGTM: Exposing this workflow via workflow_call is appropriate for snapshots.
40-46
: LGTM: Guarding publish-or-pr to only run on push meets the “push-only” requirement.
176-183
: Provenance support verified — .node-version = 22 (Node 22 / npm ≥10).node-version contains "22"; workflows use actions/setup-node@v4 with node-version-file '.node-version' and no custom npm invocation was found.
.github/workflows/snapshot.yml (1)
20-24
: LGTM: Permissions include id-token for npm OIDC.These are appropriate for the reusable publish.
- name: Publish packages with dist-tag | ||
run: pnpm publish -r --tag ${{ inputs.dist_tag }} --no-git-checks --access ${{ inputs.access }} |
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.
Add npm provenance to enable Trusted Publishers (OIDC) auth.
Without provenance, npm will expect a token and the publish will likely fail under Trusted Publishers.
Apply this diff:
- - name: Publish packages with dist-tag
- run: pnpm publish -r --tag ${{ inputs.dist_tag }} --no-git-checks --access ${{ inputs.access }}
+ - name: Publish packages with dist-tag
+ run: pnpm publish -r --tag ${{ inputs.dist_tag }} --no-git-checks --access ${{ inputs.access }} --provenance
Optionally, reinforce via env (not required if flag is present):
- - name: Publish packages with dist-tag
+ - name: Publish packages with dist-tag
run: pnpm publish -r --tag ${{ inputs.dist_tag }} --no-git-checks --access ${{ inputs.access }} --provenance
+ env:
+ NPM_CONFIG_PROVENANCE: 'true'
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
- name: Publish packages with dist-tag | |
run: pnpm publish -r --tag ${{ inputs.dist_tag }} --no-git-checks --access ${{ inputs.access }} | |
- name: Publish packages with dist-tag | |
run: pnpm publish -r --tag ${{ inputs.dist_tag }} --no-git-checks --access ${{ inputs.access }} --provenance | |
env: | |
NPM_CONFIG_PROVENANCE: 'true' |
🤖 Prompt for AI Agents
.github/workflows/publish.yml around lines 221-222: the pnpm publish command
lacks npm provenance which is required for Trusted Publishers (OIDC) flows; add
the --provenance flag to the pnpm publish invocation so npm will use provenance
instead of expecting a token, and optionally set an env var to reinforce this
(for example export NPM_CONFIG_ENABLE_OIDC=true or NPM_ENABLE_PROVENANCE=true)
before the publish step.
94c3219
to
105551a
Compare
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/publish.yml (1)
90-104
: Enable provenance for publish stepsci:release runs
pnpm publish -r --no-git-checks
(package.json:18) and there's a directpnpm publish -r ...
in .github/workflows/publish.yml (line 222); neither passes--provenance
. Add--provenance
(or setNPM_CONFIG_PROVENANCE=true
in the workflow/env) to those publish invocations and ensure OIDC/id-token or a valid token is available for provenance upload (workflow already requests id-token write).
🧹 Nitpick comments (2)
.github/workflows/publish.yml (1)
159-168
: Tighten permissions for the reusable trusted publish job.
This job doesn’t modify PRs or issues; contents write is likely unnecessary. Keep id-token: write.Apply:
trusted-publish: if: ${{ github.event_name == 'workflow_call' }} name: Trusted Snapshot Publish permissions: - contents: write # read+write repo (okay for artifacts/logs) - id-token: write # REQUIRED: OIDC for npm Trusted Publishers - issues: write - pull-requests: write + contents: read + id-token: write # REQUIRED: OIDC for npm Trusted Publishers.github/workflows/snapshot.yml (1)
21-26
: Secrets inheritance present; permissions can likely be reduced or omitted.
Called workflow defines its own permissions; the caller’s permissions aren’t required.Apply:
call-trusted-publish: name: Call trusted publish (beta dist-tag) uses: ./.github/workflows/publish.yml with: branch: ${{ inputs.branch }} dist_tag: beta prerelease: beta access: public - permissions: - contents: write - id-token: write - issues: write - pull-requests: write secrets: inherit
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/publish.yml
(2 hunks).github/workflows/snapshot.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). (1)
- GitHub Check: pr
🔇 Additional comments (6)
.github/workflows/publish.yml (5)
6-8
: Allowing workflow_call is correct for trusted publisher delegation.
Matches the snapshot wrapper design.
40-40
: Push-only guard added.
This satisfies the prior request to ensure this job runs only on push.
134-142
: Beta docs target-folder is correct.
Keeps snapshot docs isolated under beta/.
221-222
: Add npm provenance for Trusted Publishers (OIDC) auth.
Without provenance, npm will expect a token and publish will fail.Apply:
- - name: Publish packages with dist-tag - run: pnpm publish -r --tag ${{ inputs.dist_tag }} --no-git-checks --access ${{ inputs.access }} + - name: Publish packages with dist-tag + run: pnpm publish -r --tag ${{ inputs.dist_tag }} --no-git-checks --access ${{ inputs.access }} --provenance + env: + NPM_CONFIG_PROVENANCE: 'true'
176-183
: OK — workflow already meets provenance requirements. .node-version = 22 and package.json declares packageManager: [email protected], which satisfies Node 20+ (npm >= 9.5) and pnpm >= 8.6 — no changes required..github/workflows/snapshot.yml (1)
12-19
: Snapshot workflow correctly delegates to the reusable publish.
Inputs (branch, beta dist-tag/prerelease, public access) look right.
JIRA Ticket
N/A
Description
From a security perspective, we should leverage the npm trusted publishers instead of NPM tokens. This enforces publishes come from a pipeline and not anywhere else.
This also removes the use of tokens from the pipeline because there is a trusted setup between the package (on npm) and the pipeline.
A package must be published first before this can be setup, so in order to publish the package first, we should publish a beta (can do it in ci or locally).
@cerebrl to review this, please go through the packages on
npm
and verify you see the trusted publishers setup with the correct github repository & the correct workflow file (publish.yml)because we can only have 1 workflow defined as "trusted" this breaks our old "snapshot.yml".
to ensure this functionality, we have the snapshot.yml "call" the publish.yml, but a different job.
the publish-or-pr job should remain unchanged.
the publish.yml should have a trusted publish job that is specifically allowing itself to be called from another workflow (snapshot.yml) so the publish still occurs from the trusted workflow.
we can also remove tokens from this workflow.
Summary by CodeRabbit