Skip to content

ci(nixos): write the manifest from the PR's base, not the default branch - #558

Open
mrosseel wants to merge 1 commit into
brickbots:mainfrom
mrosseel:ci/manifest-checkout-base-ref
Open

ci(nixos): write the manifest from the PR's base, not the default branch#558
mrosseel wants to merge 1 commit into
brickbots:mainfrom
mrosseel:ci/manifest-checkout-base-ref

Conversation

@mrosseel

Copy link
Copy Markdown
Collaborator

One line: give update-manifest's checkout an explicit ref.

- uses: actions/checkout@v4
  with:
    ref: ${{ github.event.pull_request.base.ref }}

Why

A bare actions/checkout under pull_request_target lands on the repository's default branch — not the PR's base. So the job runs whatever .github/scripts/ the default branch happens to carry.

Today that works here by coincidence: release and main both carry the scripts. It bites in two ways anyway.

It is silently the wrong tooling. A PR based on main gets its manifest entry written by release's copy of update_manifest.py. While the two agree that is invisible — the moment the manifest schema moves on one branch it is a mismatch nobody is looking for.

It is fragile against a branch that carries the workflow but not the scripts. The workflow file has to exist on a branch for pull_request_target to fire there at all, so registering it somewhere new is exactly the thing that triggers this. In my fork I hit precisely that: a successful 19-minute build, then

bash: .github/scripts/publish_manifest.sh: No such file or directory
Process completed with exit code 127

The image was built and pushed to Attic. No device could ever see it, because the manifest was never written. A green build plus an unreachable artifact is about the worst shape a failure can take — nothing looks wrong until someone goes looking for the build in the update view.

Why the base ref specifically

It is the branch this workflow file itself came from, so its scripts are always beside it, and it stays maintainer-controlled.

Deliberately not the PR head: this job holds contents: write and the Attic token. The build jobs check out the head — that is their purpose, gated behind the testable label — but the manifest writer must not.

Testing

Running on my fork's branches since this morning; the first testable build after the change published its manifest entry correctly, where the previous run had failed at exactly this step. build-native and build-emulated still check out the PR head — verified by parsing the YAML, unchanged by this patch.

The update-manifest job used a bare actions/checkout. Under
pull_request_target that lands on the repository's DEFAULT branch, not the
PR's base, so the job runs whatever .github/scripts/ the default branch
happens to carry.

Today that works here by coincidence: both main and release carry the
scripts. Two ways it bites anyway.

It is silently the wrong tooling. A PR based on main has its manifest entry
written by release's copy of update_manifest.py. While the two agree that is
invisible; the moment the manifest schema moves on one branch it is a
mismatch nobody is looking for.

And it is fragile against a branch that carries the workflow but not the
scripts -- the workflow file has to be on a branch for pull_request_target to
fire there at all, so registering it somewhere new is exactly the thing that
sets this off. Downstream we hit precisely that: a successful 19-minute build,
then

    bash: .github/scripts/publish_manifest.sh: No such file or directory

The image was built and pushed to Attic; no device could ever see it, because
the manifest was never written. A green build and an unreachable artifact is
about the worst shape a failure can take.

Check out the PR's base branch instead. It is the branch this workflow file
itself came from, so its scripts are always beside it, and it stays
maintainer-controlled. The PR head is deliberately not an option here: this
job holds contents:write and the Attic token.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant