ci: publish canary releases - #290
Conversation
…grafana/prometheus`
This reverts commit e947853.
njvrzm
left a comment
njvrzm
left a comment
There was a problem hiding this comment.
Partial review but I'm not sure I'll be able to get back to it before your day starts, so here's my comments so far :)
| --head-sha "$HEAD_SHA" \ | ||
| --pr-number "$PR_NUMBER" \ | ||
| --run-id "$RUN_ID" \ | ||
| --run-attempt "$RUN_ATTEMPT" > "$plan_file" |
There was a problem hiding this comment.
It looks like if any of these variables is set but empty the script will run without complaint. The tests check that a missing value fails but neither the tests nor the code check for any empty value as far as I can tell. Probably not an issue but I'd be a bit happier with an explicit check.
There was a problem hiding this comment.
I checked this, and empty values were already rejected at each entry point: the Node parsers treat '' as missing, the validators reject it, and the shell wrapper uses ${VAR:?}, which also rejects set-but-empty values. I added explicit empty-value tests to both suites to better communicate this.
|
|
||
| on: | ||
| pull_request: | ||
| types: [opened, synchronize, reopened, labeled] |
There was a problem hiding this comment.
I think it's worth noting, although I'm not sure it's important for the purpose of this PR, that if main changes after this workflow runs, the published package will differ from the one that would be published after merge, if that makes sense. As long as these canary packages aren't meant to provide a definitive test of the changes that's probably fine.
There was a problem hiding this comment.
Right. The canary checks out the PR head SHA rather than the merge ref, so it represents the branch as-is and can differ from the eventual post-merge release. That's intentional, but it wasn’t documented clearly. I added a note clarifying that canaries are for trying the branch, not verifying the eventual release.
| # The name binds the artifact to this exact run, so the publish workflow | ||
| # can only ever download the tarball built by the run that triggered it. | ||
| name: npm-canary-${{ github.run_id }}-${{ github.run_attempt }} | ||
| path: ${{ runner.temp }}/npm-canary-artifact |
There was a problem hiding this comment.
I'm no GHA expert, but it looks like this will upload the entire directory as an artifact, and similarly the download later will download the directory. Not sure if I'm missing something but I don't see that being handled in the verify script.
There was a problem hiding this comment.
Yeah, this is tricky! The artifact contains the directory’s contents, not the directory itself, so the tarball lands directly in $ARTIFACT_DIR as expected. I added a comment to make that clear.
|
Thanks for the review @njvrzm! I'll comb through the feedback today. |
There was a problem hiding this comment.
Pull request overview
Adds opt-in canary publishing for @grafana/prometheus from labeled same-repository pull requests.
Changes:
- Adds unprivileged canary build and trusted publish workflows.
- Adds version planning and tarball verification scripts.
- Adds documentation, tests, and Changesets parsing dependency.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/npm-canary-build.yml |
Builds and uploads canary artifacts. |
.github/workflows/release-npm.yml |
Authorizes and publishes canaries. |
.github/scripts/verify-npm-canary-tarball.sh |
Validates artifact layout and filename. |
scripts/plan-npm-canary.js |
Computes canary versions. |
scripts/verify-npm-canary-tarball.js |
Validates package identity and version. |
scripts/__tests__/plan-npm-canary.test.js |
Tests canary planning. |
scripts/__tests__/verify-npm-canary-tarball.test.js |
Tests manifest verification. |
scripts/README.md |
Documents canary publishing. |
package.json |
Adds Changesets parser dependency. |
package-lock.json |
Locks the dependency update. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const headSha = requireCommitSha('--head-sha', options['head-sha']); | ||
|
|
||
| const plan = planCanary({ | ||
| baseVersion: JSON.parse(exec(['show', `${baseSha}:${PACKAGE_JSON}`])).version, |
| # Serialise canaries per source pull request. Manual releases fall back to the | ||
| # run ID, which is unique, so they are never queued behind one another. | ||
| group: ${{ github.workflow }}-${{ github.event.workflow_run.pull_requests[0].number || github.run_id }} | ||
| cancel-in-progress: false |
| mapfile -d '' entries < <(find "$ARTIFACT_DIR" -mindepth 1 -maxdepth 1 -print0) | ||
| if [[ "${#entries[@]}" -ne 1 || "${entries[0]}" != *.tgz ]]; then |
| if [[ "$tarball" != "$ARTIFACT_DIR/grafana-prometheus-${version}.tgz" ]]; then | ||
| echo "Error: canary tarball filename does not match the version in its manifest." >&2 |
| # contain the published code, and record that permanently in Sigstore's | ||
| # public transparency log. | ||
| NPM_CONFIG_PROVENANCE: 'false' | ||
| run: npm publish "$TARBALL" --access public --tag canary --ignore-scripts |
Motivation
This PR closes #283.
What does this PR do?
Adds opt-in npm canary publishes for the
@grafana/prometheuspackage. When we add anpm-canarylabel to a PR, the build is published under thecanarydist-tag. The workflow comments the install command on the PR.How it works
npm-canary-build.ymlis the unprivileged publish variant. It builds, derives a version from the PR's changesets, uploads a.tgzpinned to that run.release-npm.ymlis the trusted variant. It re-reads the PR, verifies the packed manifest, publishes the tarball after approval. It never runs PR code.The tarball's contents are chosen by the PR, so publishing rests on: no PR code in the OIDC job, no forks, human approval, and a manifest check requiring
@grafana/prometheusat<version>-canary.<pr>.<run-id>.<run-attempt>- values the trusted side resolved itself. That's what stops a canary passing as a release.What happens when we add new commits?
When we push a new commit to a PR with the
npm-canarylabel, or remove and re-add thenpm-canarylabel, this publishes a new canary version.Security
The tarball is packed by the unprivileged workflow, so its contents and version are chosen by the PR. Publishing relies on:
npm-canarytag to the PRverify-npm-canary-tarball.jsrequiring the manifest to name@grafana/prometheuswith a version whose prerelease identifiers are exactly the PR number, run ID and run attempt that trusted code resolved for itself. That stops a canary being published as a plain release, resolving in place of one, or being attributed to another PR.The canary version format is:
<next-version>-canary.<pr>.<run-id>.<run-attempt>.Provenance is off for canaries: npm attests the commit that ran
npm publish(amaincommit), but the tarball was built earlier, from the PR. The attestation would point at code the package doesn't contain.Before merging
release-npm.yml+npm-publish.npm-canarylabel (it's fine if this happens after merging)Testing
This PR adds 39 new tests. Some examples: accepts a valid canary, rejects a plain release version, foreign PR number, wrong package name, extra file, filename/manifest mismatch.