fix(ci): get dev green — a host-dependent test, unchecked errors, and gofmt - #14
Conversation
… gofmt
dev is currently red on lint and unit-tests, which blocks reading CI on any PR
against it. Three unrelated causes, none of them new:
1. TestSelectStorageDriver_NonComposefs asserted `driver == "vfs"` on the
premise, stated in its own comment, that "/tmp is usually tmpfs". That is a
property of the HOST, not of the code. It holds on a developer box where
/tmp is tmpfs and fails on CI runners, where /tmp is on the ext4 root and
overlay is the correct answer:
storage_driver_test.go:14: non-composefs driver = "overlay", want vfs
TestOverlayCandidate_UnsafeFilesystems right below it already guards for
exactly this; this test never got the same treatment. Rather than skip when
/tmp is not tmpfs — which would leave CI, the environment where /tmp is NOT
tmpfs, asserting nothing at all — it now detects the filesystem and asserts
the branch that applies: vfs is mandatory on tmpfs/overlayfs, and elsewhere
the driver must be one of overlay/vfs with a non-empty reason.
2. errcheck: five unchecked error returns. Four filepath.Walk calls in
internal/slurp and one exec.Cmd.Run in bootc.go. All are best-effort — the
Walk callbacks already swallow per-entry errors and a partial size/count is
the intended result, and the Run is an unmount inside a defer where a
failure has no remedy and must not change the install's outcome. Discarded
explicitly with `_ =` so the intent is visible rather than accidental.
3. gofmt -s across internal/. Mechanical, no behaviour change.
No production logic changes. go build, go vet, and the affected package tests
all pass.
… gofmt
Second pass. The first commit fixed errcheck and internal/ gofmt; unit-tests
went green and lint surfaced the rest, which my earlier reading of the log had
truncated rather than the run having changed. For the record the baseline was
already 15 nilerr, 5 errcheck, 5 gofmt, 1 unused before any of this — none of
it introduced here.
nilerr (15 sites): every one is intentional, and every one already carried an
inline comment saying so.
- internal/post: "no icons dir, skip", "no bluetooth data — nothing to do",
"no NM connections — nothing to do", and similar. These copy OPTIONAL
material into the target; the absence of a source is not an install
failure, and propagating the error would abort an install over a missing
Bluetooth directory.
- internal/slurp: returns inside filepath.Walk callbacks, where `return nil`
means "skip this entry and keep walking". A partial scan/migration is the
designed outcome, not a swallowed bug.
Annotated with //nolint:nilerr and the reason rather than restructured: the
behaviour is correct, it simply was not stated in a form the linter can read.
Each directive carries the why, so the next reader does not have to re-derive
it from the surrounding code.
writeStorageConfWithTmpDir is unreferenced. It arrived in e6ea536 and was
orphaned by 74993bb, which replaced that approach with a two-stage export.
Suppressed rather than deleted — removing code from a path that exists to work
around a live-ISO ENOSPC failure is a maintainer call, not a lint fix. If it is
genuinely dead, deleting it beats the directive, and that is a one-line follow-up.
Also gofmt -s on cmd/, which the first pass missed by only covering internal/.
go build, go vet, gofmt -s and the affected package tests all pass.
…t build`
With lint and unit-tests green, the VM matrix finally ran — and every job in
it, required and advisory alike, failed identically:
/tmp/justHS63lX/build: line 67: go: command not found
error: Recipe `build` failed with exit code 127
The cause is step ordering. actions/setup-go installs the toolchain into
/opt/hostedtoolcache, and the very next step runs jlumbroso/free-disk-space
with tool-cache: true, which deletes that directory. Go was being installed and
then thrown away before anything could use it.
This was never observed because lint and unit-tests were failing first, so the
whole matrix was skipped on every run. Fixing those two exposed a second,
entirely separate pre-existing breakage underneath.
Moving the cleanup ahead of setup-go fixes it and is strictly better than
setting tool-cache: false: the jobs still reclaim the full ~30 GiB they need
for multi-GB OCI pulls and a target loop disk, and Go survives because it is
installed after the sweep rather than before it.
Applied to both vm-boot-required and vm-boot-advisory, which had the same
ordering.
Status: lint and unit-tests are green. The VM matrix is a separate, deeper problem — and I think #11 is its fix.Fixed here (all pre-existing on
Making those green exposed a second breakage that had never been observed, because the matrix was skipped on every run while lint/unit-tests were red. Every VM job — required and advisory — died identically:
What remains, and why I stoppedWith the build fixed, the jobs now get 4–9 minutes in (vs ~3 failing at build) and fail in the actual test: The image itself exists and is pullable from ghcr — I checked. The failure is that the qualified That is precisely what #11 describes: "the image pull succeeds into redirected, disk-backed Podman storage, but OCI export must address that exact store" — and it touches So I have deliberately not attempted to fix this. Reimplementing an open PR of someone else's to chase a green tick would be the wrong move, and #11 landing first is very likely the real answer — at which point this branch's matrix becomes the validation #11 has been waiting for. Also worth noting: Suggested order: merge this (it is self-contained, changes no production logic, and makes CI readable), then #11, then #13 — whose own run is currently meaningless against a red baseline. |
1 participant
devis currently red onlintandunit-tests, which makes CI unreadable on every PR against it — a new PR looks broken whether or not it is. Three unrelated causes, none of them new.1. A test that asserts a property of the host, not the code
TestSelectStorageDriver_NonComposefsasserteddriver == "vfs"on the premise stated in its own comment — "/tmp is usually tmpfs". That is true on a developer box and false on your CI runners, where/tmplives on the ext4 root andoverlayis the correct answer:TestOverlayCandidate_UnsafeFilesystems, directly below it, already guards for exactly this with at.Skipf; this test never got the same treatment.I did not fix it by skipping. Skipping when
/tmpis not tmpfs would leave CI — precisely the environment where/tmpis not tmpfs — asserting nothing at all, which is where the regression risk actually lives. Instead it detects the filesystem and asserts the branch that applies:tmpfs/overlayfs,vfsis mandatory (those cannot back overlay)overlay/vfsand carry a non-empty reason, because whether podman can set up overlay there is genuinely environment-dependent and both answers are correct2. errcheck — five unchecked error returns
Four
filepath.Walkcalls ininternal/slurpand oneexec.Cmd.Runinbootc.go. Every one is genuinely best-effort:Walkcallbacks already swallow per-entry errors, and a partial size/count is the intended resultRunis an unmount inside adefer, where a failure has no remedy and must not change the install's outcomeSo the fix is
_ =rather than error propagation — the behaviour was already correct, it just was not stated. Each has a comment saying why discarding is right, so the next reader doesn't have to re-derive it.3. gofmt -s
Mechanical across
internal/. No behaviour change.Verification
go build ./...,go vet ./internal/..., and the affected package tests all pass. No production logic changed.Why this is separate from #13
#13 (the ext4 verity fix) is a real behaviour change and deserves to be read on its own. This PR only makes
devgreen so that #13 — and anything else — can be judged on its own CI rather than inheriting a red baseline. Merging this first makes #13's run meaningful.Two failures I did not touch, because they do not reproduce in your CI:
TestBuildSelinuxBypassShim_InterceptsSecuritySelinuxandTestBootcInstall_NonComposefsContainerExportsOCIboth fail on my local machine against unmodifieddevbut pass on your runners. They look environment-dependent in the opposite direction, and are worth a separate look rather than a speculative fix from me.🤖 Generated with Claude Code
https://claude.ai/code/session_01VvyH2muGbDWLmmmtrPNkwG