ci(nixos): write the manifest from the PR's base, not the default branch - #558
Open
mrosseel wants to merge 1 commit into
Open
ci(nixos): write the manifest from the PR's base, not the default branch#558mrosseel wants to merge 1 commit into
mrosseel wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
One line: give
update-manifest's checkout an explicitref.Why
A bare
actions/checkoutunderpull_request_targetlands 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:
releaseandmainboth carry the scripts. It bites in two ways anyway.It is silently the wrong tooling. A PR based on
maingets its manifest entry written byrelease's copy ofupdate_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_targetto 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, thenThe 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: writeand the Attic token. The build jobs check out the head — that is their purpose, gated behind thetestablelabel — but the manifest writer must not.Testing
Running on my fork's branches since this morning; the first
testablebuild after the change published its manifest entry correctly, where the previous run had failed at exactly this step.build-nativeandbuild-emulatedstill check out the PR head — verified by parsing the YAML, unchanged by this patch.