CLOUDPLAT-3162: add npm OIDC publish workflow (s3scan)#36
Conversation
|
Successfully scanned changes introduced in a pull request into Internal scan identifier:
See all issues found during this scan in the OX Security Application. Detailed information
1 aggregation
|
| ### Steps | ||
|
|
||
| 1. **Bump the version** in `package.json` (follow [semver](https://semver.org)) | ||
| 2. **Update `CHANGELOG.md`** with a summary of what changed |
There was a problem hiding this comment.
There's no CHANGELOG.md in most of the repos this PR template was submitted to — the more common practice is to maintain changelogs in the GitHub Releases section, so this item doesn't make sense.
| "underscore": "^1.13.6" | ||
| }, | ||
| "publishConfig": { | ||
| "access": "public" |
There was a problem hiding this comment.
The workflow already passes the public tag (npm publish --access public), so this change is fully redundant.
| { | ||
| "name": "@mapbox/s3scan", | ||
| "version": "3.0.0", | ||
| "version": "3.0.1", |
There was a problem hiding this comment.
There is zero reason to bump versions in this PR template because changing the workflow does not change the package contents itself — instead of asking every receiver of these PRs to "remove the bump if you don't need it" (which is a lot of manual labor), it should have been the other way around (but I really can't think of any reason such a PR would need a version bump, except to test out the workflow immediately with a vain version).
|
|
||
| jobs: | ||
| npm-release: | ||
| uses: mapbox/gha-public/.github/workflows/workflow-npm-oidc-publish.yml@main |
There was a problem hiding this comment.
ox-security is right — it's fragile to have this unpinned / not versioned; even an inconspicuous-looking update of the workflow can break releases across hundreds of repos.
| id-token: write | ||
| contents: write | ||
| with: | ||
| create-github-release: true |
There was a problem hiding this comment.
This might make sense for private packages, but it's a questionable default for public projects, where it's important to have release notes written with care, not auto-generated from commits.
|
|
||
| ### Steps | ||
|
|
||
| 1. **Bump the version** in `package.json` (follow [semver](https://semver.org)) |
There was a problem hiding this comment.
If we want to follow common practices, a release should be 1) tagged, 2) marked with a clear semver commit. So npm version is a more universal instruction.
|
|
||
| The workflow will publish to npm and create a GitHub release with auto-generated notes. | ||
|
|
||
| > **Note:** Only Mapbox maintainers with write access to this repository can trigger the release workflow. External contributors can open and contribute to PRs, but releases are always cut by the owning team. |
There was a problem hiding this comment.
This whole note feels redundant. Does it really need clarification that external PR authors can't publish NPM releases by themselves, and only package maintainers can actually do that?
|
|
||
| jobs: | ||
| npm-release: | ||
| uses: mapbox/gha-public/.github/workflows/workflow-npm-oidc-publish.yml@main |
There was a problem hiding this comment.
Also a few comments on the workflow itself to keep my feedback in one review:
- Node version should be v24, the current LTS. v22 is in maintenance mode. Subsequently, there'd be no need to do
npm install -g npm@^11.5.1because v24 comes with v11.6.0+ bundled. - "Run tests" and "Build package" should not be in the workflow. Gating and preparing assets before publish is the responsibility of
prepublishOnlyhook inpackage.json. - "Build package" runs unconditionally, which makes it run twice if a package does have
prepublishOnlyset up properly.
There was a problem hiding this comment.
In general this workflow seems redundant as a whole. You have a 146-line boilerplate running typical workflow things like npm ci and tests with a lot of verbose options, but there's no value in it being reusable because you can replace the composition of this uses workflow and the dependency with a compact 17-line self-sufficient workflow that is much easier to maintain and less fragile. This push was about switching to OIDC publishing specifically, so it doesn't warrant pushing projects towards more complicated workflows with logic unrelated to OIDC.
Summary
Adds the reusable
workflow-npm-oidc-publishworkflow frommapbox/gha-public.This replaces local/manual
npm publishwith a GitHub Actions workflow using OIDC Trusted Publishing — no npm tokens required.Trigger: manual (
workflow_dispatch) — run from the Actions tab to publish.Ticket: https://mapbox.atlassian.net/browse/CLOUDPLAT-3162