feat(devcontainer): derive the base-image pin instead of bumping it - #225
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #225 +/- ##
==========================================
+ Coverage 84.07% 84.45% +0.37%
==========================================
Files 39 40 +1
Lines 2129 2193 +64
Branches 91 99 +8
==========================================
+ Hits 1790 1852 +62
- Misses 324 325 +1
- Partials 15 16 +1
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
The Dockerfile's FROM digest was modelled as a dependency: publish the
image, wait for Renovate to notice, merge a bump. But the digest is a pure
function of the source -- pkg_tar fixes timestamps and oci_push publishes
the exact index Bazel assembled -- so it is knowable in the PR that changes
the image, and the round-trip only bought a window where the pin named a
superseded image. Verified: a local build of the merged tree produced
sha256:0f31f38a..., byte-identical to what CI published.
It becomes a derived file, in the sense MODULE.bazel.lock already is:
- sync_base_image_pin.py rewrites the pin from the built layout index.
- //.devcontainer:test_base_image_pin fails when it drifts. Not a
pre-commit hook: the digest falls out of an already-built Bazel
artifact and needs no daemon, so it rides `bazel test //...`.
- renovate-derived-files.yml re-derives it when MODULE.bazel moves the
upstream base, in the same commit as the other derived files, and
after `bazel mod deps` -- that step needs a cold output base and this
build would warm it.
- Renovate is told to ignore the dep, so no bump PR can restate what
the tree already determines.
The trade, documented at each site: on a branch that edits the base, and
on main until publish finishes, the pin names an image the registry does
not have yet. DEVCONTAINER_BASE_IMAGE pointed at the published :latest is
the way through. Consumers outside this repo (.dotfiles) cannot derive
anything and keep the Renovate-bumped pin.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… test Deriving the pin only closed half the loop: the CI test reported drift and left you to run the command by hand, where MODULE.bazel.lock and uv.lock both get a hook that fixes it in place. The pin now gets the same treatment, and for the same reason -- a derived file nobody has to remember to regenerate. `files` covers every input to the image rather than the base directory alone: MODULE.bazel carries both the upstream pull and the rules_oci / rules_pkg versions that assemble it, and .bazelversion is in that class too. The test stays, and is not redundant. Hooks do not run for --no-verify, a web edit, or the helper app`s API commits, and they do not re-run when a branch rebases onto someone else`s base change -- two individually fresh pins can be jointly stale. Verified both ways against a corrupted pin: the hook rewrites it and fails the commit, the test fails on its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rive it The two classifications disagreed about what feeds the image. renovate-derived-files.yml counts .bazelversion as a bazel change and re-derives the pin from it; devcontainer.yml did not, so the merge would rebuild and publish nothing. A bazel bump that moved the digest would have written a pin naming an image no job ever pushes: the consumer build cannot pull it, and no amount of re-running fixes a red PR whose failure is a config gap. Latent until now -- a bazel release changing rules_oci output is unlikely, and until the pin was derived nothing would have moved. Deriving it is what makes the asymmetry reachable, so it is worth closing in the same PR rather than leaving a trap for whoever meets it first. Cost is a no-op republish per bazel bump, which is what a MODULE.bazel edit that misses the image already costs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The hook and renovate-derived-files.yml both run before the merge, so neither can see the step that fails after it. `crane push` followed by one `crane tag` per tag is not atomic: a failure in between leaves main pinned to a digest the registry does not serve, or tags trailing a digest that is. Both are silent until somebody rebuilds a devcontainer and cannot pull its base. The publish job now reads the pin back and checks it three ways: the manifest resolves by digest (what a consumer FROM needs), and :latest and sha-<gitsha> both point at it (what Renovate and any non-deriving consumer read). A half-publish becomes a red main, which is recoverable by re-running -- the documented remedy, now with something that says when to apply it. The pin is read through sync_base_image_pin.py --print-pinned rather than a grep local to the workflow, so there is one parser for the Dockerfile shape and no second copy to degrade into a check that always passes. That mode returns before the layout check because the publish job has no Bazel output tree; a test pins that, and a mutation making it print nothing is caught. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
10ffff1 to
a13fce2
Compare
|
Rebased onto
The test stays and isn't redundant: hooks don't run for
On running pre-commit in a CI job as a general backstop: it would work mechanically — Verification: rebased tree derives One housekeeping item this doesn't do: #224 (Renovate's bump of the pin) should be closed by hand once this merges — the |
The root README described the pin as written by sync_base_image_pin.py and checked by bazel test, which was the shape before the hook existed. The script is what writes it; the hook is what runs the script, and for an edit made here that is the whole mechanism. Rewraps a future-considerations line this branch left at 149 characters. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The pin got a backstop in this PR while the other two fixers kept relying on the hook having run. Both are now checked in CI, fail-only: the regenerated file belongs in the authoring commit, not in a CI push. MODULE.bazel.lock had nothing at all -- builds run with --lockfile_mode=update, which rewrites the lock in memory and stays green, so a bypassed `bazel mod tidy` was invisible until it blocked someone else`s commit. A new ci.yml job runs `bazel mod tidy` and diffs MODULE.bazel and the lock. Verified by staging a lock with a dropped registry hash: the job`s own commands exit 1. uv.lock was half-covered. check_uv_lock_fresh compares requirements_lock against the lock, which stays consistent while the *lock* drifts from pyproject -- exactly what a bypassed hook leaves. check_uv_lock_current adds `uv lock --check`, which validates the existing resolution rather than redoing it (sub-millisecond here), so this is not the full re-lock that was deliberately kept out of CI. That new check immediately caught a defect in the old one: `uv export` updates uv.lock before exporting, so check_uv_lock_fresh was silently re-locking the tree it was checking. In CI that repairs staleness inside the runner and then compares against a file the commit does not contain. Fixed with --frozen, pinned at the argv seam next to the other flags. The pre-commit hook re-locks before exporting and needs no such flag. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Syndic
left a comment
Syndic
left a comment
There was a problem hiding this comment.
The core premise holds: pkg_tar fixes mtimes, oci_push publishes the assembled index, and the layout index.json → single-manifest indirection is the one test_image_layers.py already walks. //meta/devcontainer-base:image was already a data dep of that test, so the new guard adds no build cost to bazel test //.... The mutation testing in the description is the right evidence for a rewriter that can brick every devcontainer, and anchoring _PIN_RE on the repository so COPY --from=ghcr.io/astral-sh/uv:… can't match — with a test for it — is the kind of thing that's only obvious after it bites someone.
Fifteen inline notes below. Four I'd want addressed before merge:
- the merged paragraphs and the now-false cache rationale in
meta/devcontainer-base/README.md - the two comments that still claim the
uv exportinvocation mirrors the pre-commit hook "exactly", which--frozenhas just made false on both sides main()'s rewrite path — the mutating half both the hook and the workflow invoke — having no test
The rest are judgment calls, cost observations, or follow-ups.
One scope note: bazel-lock-check and check_uv_lock_current are unrelated to the base-image pin. The justification (same gap in sibling derived files) is good and the --frozen bug they surfaced is a real find — flagging only because it makes this harder to bisect later.
Behaviour: - The publish verification was single-shot against a registry whose tag->digest resolution is not documented as read-your-writes, seconds after `crane tag`. Three attempts with backoff, and one tool for all three references -- `imagetools inspect` takes a digest reference too, and `docker manifest` is still nominally experimental. Each attempt is wrapped in `timeout`: a call that never returns would hang the publish job on main. - A stale uv.lock produced two errors for one cause, the second naming requirements_lock.txt. The two uv checks are now sequential, and a test pins that the export diff does not run when the lock itself is stale. - Dropped `--check` from sync_base_image_pin. No caller, no test, and the module docstring advertised it -- the shape that rots. The bazel test is the check, and reads the same pure functions. - `//meta/devcontainer-base:image` is visible to `//.devcontainer` rather than public, which is what its own comment already claimed. Tests: main()`s rewrite path is covered -- rewrite, no-op re-run, and a missing layout returning 2 without touching the Dockerfile, the case with no backstop anywhere. test_devcontainer_config takes the digest half of its assertion from sync_base_image_pin instead of carrying a second sha256 regex. Comments and docs, all of which said something that stopped being true: the export "mirrors the hook exactly" (it deliberately differs by --frozen, in two places), the cache paragraph blaming a later digest bump that no longer exists, a missing blank line joining two paragraphs, a 175-character line, the base-image-pin hook`s ability to rewrite MODULE.bazel.lock and be blamed for it, and which cost the .bazelversion classification takes and why. The new CI job and the pre-existing shellcheck job are now in the README table. `bazel mod tidy --config=ci` matches gazelle-check next door; verified the config resolves for `mod`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
All fourteen addressed in 8e7e87a. Taken as-is except one, noted below. Publish verification — both points taken. Three attempts with backoff, and Two errors for one cause — the uv checks are sequential now, and there's a test asserting
Two Dockerfile parsers — took the dedup rather than narrowing the claim. Visibility —
Comments and docs — all corrected: the "mirrors the hook exactly" claim in both places it appeared, the cache paragraph (rewritten to the surviving reason —
|
Two leftovers from the review pass. The publish verification dropped stderr, so propagation lag, an auth failure and an unreachable registry all reached the log as the same empty answer -- and the annotation tells the reader to re-run the job, which only helps for the first of those. The last stderr now follows the ::error:: line. The .bazelversion bullet reflowed only as far as the long line that needed it, leaving a 29-character orphan mid-paragraph. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The retry loop slept its full backoff after the last attempt, then fell straight out and failed -- 15s per ref, three refs, buying nothing. It also read as though the sleep belonged to an attempt rather than to the gap between two. Guarded with `||` rather than `&&` deliberately. An `&&` guard whose test fails returns 1; that survives set -e as the last line of a loop body but not as the last line of a function, so the working form here would have been a property of where it sits. The `||` form returns 0 either way. The retry count was already spelled twice (the loop list and the error text, silently coupled); a guard would have made it three. Hoisted to `attempts` so the three uses cannot drift. Exercised the failure path with stubs, which CI has never run: two sleeps for three attempts, the count interpolated into the message, and a stubbed `unauthorized` reaching the log rather than /dev/null. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tempt A `timeout` kill writes nothing to stderr -- GNU coreutils confirms 0 bytes and exit 124 -- so that failure printed the stderr header followed by silence. It was the one case a reader could not name: empty stderr plus an empty answer looked identical to a docker failure that happened to print nothing. Capturing the status with `|| rc=$?` closes it, and replaces the `|| true` that was there for set -e. rc is reset at the top of each attempt, not only captured. `|| rc=$?` assigns on failure alone, so without the reset a run where attempt 1 timed out and attempts 2-3 exited 0 with a mismatched digest would have reported 124 for an attempt that exited cleanly -- reintroducing the misleading diagnostic this is meant to remove. The second `rc=0`, beside `resolved=""`, is the set -u guard for if `attempts` ever stops being a literal. Exercised all three with stubs, none of which CI runs: a timeout kill reports 124, an auth failure reports 1 with its message, and the stale-rc case reports 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The `(124 = timed out)` legend printed on every failure, so an auth failure read `Last attempt exited 1; its stderr follows (124 = timed out):` -- a legend the reader has to decode mid-sentence, in a step whose whole purpose is saying plainly what went wrong. Now the parenthetical appears only when rc is actually 124. `||` rather than `&&` for the same set -e reason as the sleep guard above it. Re-ran the three stub paths: 124 gains ` (timed out)`, exit 1 and exit 0 carry no parenthetical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Follow-up to #222. Supersedes #224 — its one-line digest change is included here, arrived at by construction rather than by Renovate noticing.
Why
I modelled the Dockerfile's
FROMdigest as a dependency: publish the image, wait for Renovate, merge a bump. It is really a derived file, in the same sense asMODULE.bazel.lock,requirements_lock.txtanddevcontainer-lock.json— all of which this repo already regenerates in the commit that invalidates them.The digest is a pure function of the source tree:
pkg_tarfixes timestamps, andoci_pushpublishes the exact index Bazel assembled rather than rebuilding it. Confirmed against the artifact #222 published:So the round-trip bought nothing except a window — one publish plus one Renovate cycle — in which the pin named a superseded image, and a standing question about whether to automerge the bumps.
What changed
meta/scripts/sync_base_image_pin.pyrewrites the pin from//meta/devcontainer-base:image's layout index. Pure functions, unit-tested; it refuses to guess when theFROMshape isn't exactly what it expects.base-image-pinpre-commit hook rewrites it for our own edits, the same waybazel mod tidyanduv lockdo for the other derived files.//.devcontainer:test_base_image_pinfails when the pin drifts, printing the expected digest and the command to fix it. It is the backstop under the hook, not a replacement for it — hooks don't run for--no-verify, a web edit, or the helper app's API commits, and don't re-run on a rebase. A plainbazel testrather than a CI job because the digest falls out of an already-built artifact and needs no daemon.renovate-derived-files.ymlre-derives the pin whenMODULE.bazelmoves the upstream base, committing it alongside the other derived files. It runs afterbazel mod deps, never before: that step only rewrites the pipfactson a cold output base, and this build would warm it.--config=local, since that job carries no BuildBuddy key.renovate.jsondisables the dep, so no bump PR can restate what the tree already determines.The trade-off
On a branch that edits
meta/devcontainer-base/, and onmainuntil the publish job finishes a couple of minutes after merge, the pin names an image the registry doesn't have yet. The way through isDEVCONTAINER_BASE_IMAGEpointed at the published:latest, which I verified builds:DEVCONTAINER_BASE_IMAGE=ghcr.io/syndic/unnatural_designs-devcontainer-base:latest devcontainer build --workspace-folder .That runs the previous plumbing locally; validating the candidate is CI's job, via
bazel run :load. Worth noting I documented:loadas the local recipe first and then corrected it — it needs Bazel and a Docker daemon in the same place, which in this repo means the host, contradicting the devcontainer-only rule.One more consequence, in the good direction:
BASE_IMAGEstops being a nice-to-have. On a base-changing PR the pinned digest genuinely doesn't resolve, so a broken override fails the build loudly instead of silently smoke-testing the old base.This does not transfer to
Syndic/.dotfiles. It doesn't build the image, so for it the pin stays a real dependency with a Renovate bump — documented in the base README so phase 5 doesn't copy the wrong half.Verification
0f31f38a…, and re-running is a no-op (--checkagrees).replace_pin;_matchrelaxed to take the first of several pins.main's pin was stale by exactly the digest chore(deps): update ghcr.io/syndic/unnatural_designs-devcontainer-base:latest docker digest to 0f31f38 - autoclosed #224 proposed.bazel test //...21/21, gazelle clean,ruff/ty/shellcheck/check_modules.pypass.Reviewer notes
//meta/devcontainer-base:imagegains public visibility so the consumer can assert against it.renovate-derived-files.ymla cold image build onMODULE.bazelPRs (two shell scripts over a pulled base).🤖 Generated with Claude Code