Skip to content

CLOUDPLAT-3162: add npm OIDC publish workflow (s3scan) #36

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.

Sign up for GitHub

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
haseebehsan merged 2 commits into from
Jun 22, 2026
+41 −1
Merged

CLOUDPLAT-3162: add npm OIDC publish workflow (s3scan) #36

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b7173b4
CLOUDPLAT-3162: add npm OIDC publish workflow
haseebehsan Jun 18, 2026
fe15526
CLOUDPLAT-3162: add CONTRIBUTING.md and bump version
haseebehsan Jun 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions .github/workflows/npm-release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
name: NPM release

on:
workflow_dispatch:

jobs:
npm-release:
uses: mapbox/gha-public/.github/workflows/workflow-npm-oidc-publish.yml@main

Check warning on line 8 in .github/workflows/npm-release.yml

View check run for this annotation

OX Security / ox-security/scan

Unpinned Reusable Workflow • GitHub Actions 

Pin reusable workflows to a full-length commit SHA (40 characters) instead of a tag or branch. Example: uses: org/repo/.github/workflows/build.yml@a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6e7f8a9b0

@mourner mourner Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mourner mourner Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.1 because 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 prepublishOnly hook in package.json.
  • "Build package" runs unconditionally, which makes it run twice if a package does have prepublishOnly set up properly.

@mourner mourner Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

permissions:
id-token: write
contents: write
with:
create-github-release: true

@mourner mourner Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

24 changes: 24 additions & 0 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Contributing to s3scan

## Development

```bash
npm ci
npm test
```

## Releasing a new version

Releases are published to npm via GitHub Actions.

### Steps

1. **Bump the version** in `package.json` (follow [semver](https://semver.org))

@mourner mourner Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

2. **Update `CHANGELOG.md`** with a summary of what changed

@mourner mourner Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

3. **Open a PR**, get it reviewed and merged to `master`
4. **Trigger the release** from the [Actions tab](../../actions/workflows/npm-release.yml):
- Select **NPM release** → **Run workflow** → run from `master`

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.

@mourner mourner Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

5 changes: 4 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@mapbox/s3scan",
"version": "3.0.0",
"version": "3.0.1",

@mourner mourner Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"description": "Streaming operations on S3 objects",
"main": "index.js",
"engines": {
Expand Down Expand Up @@ -39,5 +39,8 @@
"eslint": "^8.52.0",
"tape": "^4.6.3",
"underscore": "^1.13.6"
},
"publishConfig": {
"access": "public"

@mourner mourner Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The workflow already passes the public tag (npm publish --access public), so this change is fully redundant.

}
}
Loading