Skip to content
This repository was archived by the owner on Sep 17, 2026. It is now read-only.

Move the test image pins out of test bodies and into testutil - #19

Merged
Peeja merged 1 commit into
mainfrom
claude/test-image-pins
Sep 17, 2026
Merged

Peeja merged 1 commit into
mainfrom
claude/test-image-pins

Conversation

@Peeja

@Peeja Peeja commented Sep 17, 2026

Copy link
Copy Markdown

Answers your review question on #17. Stacked on it, because it moves the exact lines #17 touches — unstacked, whichever merged second would conflict.

Two new files, four one-line call-site changes. No git subtree add, so rule 8 does not apply.

You were right, and the repo already agreed with you

The strings pre-date #17 — it only appended @sha256:… to literals already inline, so the placement was upstream's. But the instinct holds, and piri/pkg/internal/testutil/minio.go is this repository's own better answer: a named const, a doc comment saying why the pin is what it is, and an environment override for pointing at a local build.

indexing-service/pkg/internal/testutil/images.go   MinioImage(), ValkeyImage()
swarf/internal/testutil/images.go                  PostgresImage()

hilt and sprue are halfway there — raw literal, but in a testutil package. The four sites here were the least-good shape in the repository and are now the best one.

Why exactly four, rather than a line I drew

I surveyed all 19 pinned image references in Go across six modules before deciding:

where count verdict
already in a testutil package 7 leave — hilt, piri ×3, sprue ×2
smelt production code 4 leave — smelt generates compose files; the image belongs there
smelt build_test.go heredocs 3 leave — FROM alpine:… is test input, not an image the test pulls
ingot/itest awsCLIImage 1 leave — already a named const, with five lines on why the CLI version is the surface under test. Better than anything I'd impose
inline literal passed to a Run call 4 moved

So "scope it to the duplication rather than these four lines" turned out to be the same four lines. I said that differently before checking; the survey is what settled it.

Two things I deliberately did not do

No shared cross-module constant. postgres:16-alpine@sha256:cf78e766… is named in eight places across five modules (I told you six earlier — the survey says eight). A single source of truth needs a shared package, which across five separate Go modules means a new in-repo module plus five replace edges, to carry a test constant. That cure is worse than the disease. Renovate groups them instead — once it is installed.

No guard, for the reason #17 already gives. "No image literal in a _test.go" is clean and list-free, but it can only see references that are already pinned — a 64-hex digest is unambiguous, an unpinned image is not. It would enforce placement while reading as though it enforced pinning. That is exactly the partial-guard shape this consolidation keeps deleting.

One thing left on the table

swarf's two call sites are otherwise identical — the same twelve lines of database, user, password and wait strategy, twice. A RunPostgres(t) helper would remove real duplication, but that is a different refactor with a different justification, and it is not what you asked about. Say the word.

Verified

go vet clean on indexing-service and swarf; gofmt -s -l . reports 0 files repo-wide; both existing guards still pass (26 base images, 15 compose images); and no _test.go in the tree holds an image literal any more except ingot's documented const.

🤖 Generated with Claude Code

https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt


Generated by Claude Code

@Peeja
Peeja added this pull request to stack #18 September 17, 2026 14:59
Peeja pushed a commit that referenced this pull request Sep 17, 2026
The wiki listed #15 as open after it merged and did not mention #19 at all.
Petra noticed before the next check-in did, which is the point: the wiki has
been updated when a scheduled check-in happened to fire, not when the thing it
describes changed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
@Peeja
Peeja force-pushed the claude/test-image-pins branch from 5514f94 to f489d7d Compare September 17, 2026 15:00
Peeja pushed a commit that referenced this pull request Sep 17, 2026
main is 3c3fe76. #17 and #19 remain, stacked. The rename landing means a
branch protection rule still naming `replaces` is now waiting on a check that
will never report -- noted where someone looking for blockers will find it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
Base automatically changed from claude/pin-stragglers to main September 17, 2026 15:21
Answers Petra's review question on #17: putting these strings directly in Go
test files feels out of place. It is, and this repository already has a better
answer of its own.

**The strings pre-date #17**, which only appended `@sha256:...` to literals
that were already inline; the placement is upstream's. But the instinct is
right, so this moves them.

**piri's shape, copied deliberately.** `piri/pkg/internal/testutil/minio.go`
has a named const, a doc comment saying why the pin is what it is, and an
environment override for pointing at a local build. hilt and sprue are
halfway there -- raw literal, but in a testutil package. The four sites here
were the least-good shape in the repository, and are now the best one.

    indexing-service/pkg/internal/testutil/images.go   MinioImage, ValkeyImage
    swarf/internal/testutil/images.go                  PostgresImage

Scoped to four sites, and the survey is why rather than an arbitrary line.
Nineteen pinned image references exist in Go across six modules. Seven are
already in testutil packages. Four are in smelt production code, where they
belong -- smelt *generates* compose files. Three more are in smelt's
build_test.go, inside heredoc Dockerfiles: test *input*, not images the test
pulls. One is ingot's awsCLIImage, already a named const with five lines
explaining why the CLI version is the surface under test -- better than
anything this change would impose. That leaves exactly these four.

**swarf's two call sites remain otherwise identical** -- same twelve lines of
database, user, password and wait strategy, twice. Extracting a RunPostgres
helper would remove real duplication, but that is a different refactor with a
different justification, and it was not what the review asked about.

**No shared cross-module constant.** postgres:16-alpine@sha256:cf78e766 is
named in eight places across five modules. A single source of truth needs a
shared package, which for five separate Go modules means a new in-repo module
and five replace edges to carry a test constant. That is worse than the
problem. Renovate groups them instead -- once it is installed.

**No guard, for the reason #17 gives.** A rule like "no image literal in a
_test.go" is clean and list-free, but it can only see references that are
already pinned: a 64-hex digest is unambiguous, an unpinned image is not. It
would enforce placement, not pinning, while reading like it enforced both.
That is the partial-guard shape this repository has been removing.

Verified: go vet clean on indexing-service and swarf, gofmt -s clean
repo-wide, both existing guards still pass, and no `_test.go` in the tree
holds an image literal except ingot's documented const.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
@Peeja
Peeja force-pushed the claude/test-image-pins branch from f489d7d to 6442c4c Compare September 17, 2026 15:21
@Peeja

Peeja commented Sep 17, 2026

Copy link
Copy Markdown
Author

LGTM; merging when green.

Peeja pushed a commit that referenced this pull request Sep 17, 2026
Two pages of state that moved with them: the open list is #19 and #21, both
on main and independent now that #17's merge retargeted #19 and Petra rebased
it; the branch-deletion list is eighteen rather than seven, derived rather
than typed; the root AGENTS.md exists, so the thing that was proposed is now
the thing that either works or does not.

#21 does not need rebasing -- it touches only itest.yml and MONOREPO_TODO.md,
neither of which main has touched since 3c3fe76.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
Peeja pushed a commit that referenced this pull request Sep 17, 2026
Run 35240022926 is green on all four jobs, and #19's unsharded run finished
four minutes later on the same runner pool, so there is a real A/B rather than
a comparison against last night:

    itest workflow      30m43s unsharded   ->  23m07s sharded
    slowest job         29m29s             ->  17m07s
    test binary         1267.853s          ->  628.841 + 462.731 + 204.654

Total test work is unchanged (+2.2%), which is the point: nothing got cheaper,
it got spread. But the saving is ~25%, not the "roughly half" this entry
claimed, and the three reasons are all things the 13-uniform-boots model did
not have.

The shards are unbalanced 629/463/205 because round-robin splits by test name
order and assumes uniform cost. Shard 1 drew TestForgeVersity, whose subtests
include dozens of three-second retention waits, plus two more TTL-bound tests.
Perfect balance would be 432s, so imbalance alone costs 3m17s. On top of that
the shards waited 2m25s-4m47s for runners, and the ~6 minute image build is
per job and did not move.

That also falsifies the boot decomposition twice-quoted here. Shard 3 ran four
tests -- four boots -- in 204.654s, so a boot is at most ~51s, not ~80s, and
the 1040s-booting/217s-working split was wrong. The shared stack is worth less
than the 3-4 minutes last estimated, not more, so it is parked and building the
images once is now unambiguously the largest lever.

Balancing the shards by measured duration is the one remaining cheap win, and
it has to be derived from a previous run's timings rather than hand-grouped --
a hand-written grouping is a list a new test falls out of silently, which is
the shape this repository keeps deleting.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
Peeja pushed a commit that referenced this pull request Sep 17, 2026
#21's run is green on all four jobs and #19's unsharded run finished four
minutes later on the same pool, so this is an A/B rather than a comparison
against last night: 30m43s -> 23m07s on the workflow, 1267.853s of test binary
-> 628.841 + 462.731 + 204.654. Total test work unchanged at +2.2%, which is
the point -- nothing got cheaper, it got spread -- and runner time went 29m29s
-> 42m04s.

The missing half is three things: shards unbalanced 629/463/205 because
round-robin splits by name order and TestForgeVersity is enormous; 2m25s-4m47s
of queue wait for four concurrent jobs instead of two; and the ~6 minute image
build, which is per job and did not move.

Shard 3 ran four tests, so four boots, in 204.654s. A boot is therefore at most
~51s, not the ~80s this page twice derived from 1257/13, so the
1040s-booting/217s-working split was wrong and the shared stack is worth less
than last estimated rather than more. Parked. Building the images once is the
largest lever by a clear margin now.

Also: the open-PR row for #21 now names 6fb033c, the docs-only commit carrying
this measurement.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
Peeja pushed a commit that referenced this pull request Sep 17, 2026
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
Peeja pushed a commit that referenced this pull request Sep 17, 2026
Petra's idea: since nothing is required yet, say per PR which checks a reviewer
can merge without waiting for, with reasoning a paths: filter could not express.
It is the interim for the path-filtering question and avoids both of that
question's faults -- a list that goes stale silently, and skipped jobs that
never satisfy a required check.

Two conditions make it real rather than comfortable: derive the claim (the
closure is not obvious, which is why CI is unfiltered at all), and state what it
costs when wrong (the checks still run, so an ignored red leaves main red). The
blocks are kept as the worked examples that design the real filtering later.

#22 puts it in AGENTS.md. Also: #19 is green, #21 is at e020534.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
@Peeja
Peeja merged commit a7f3494 into main Sep 17, 2026
22 checks passed
@Peeja
Peeja deleted the claude/test-image-pins branch September 17, 2026 16:22
Peeja pushed a commit that referenced this pull request Sep 17, 2026
The page said "itest ingot is sharded" as of the last commit, which stopped
being true at 16:21Z. Fixed everywhere: the open-PR tables are #19 and #22, the
second outstanding check-name change is off the table, and the debt entry now
leads with ~30 minutes unsharded as a measured choice rather than an unexamined
one.

The measurement is kept in full, because every remaining option is judged
against it and because reviving the branch is a reopen rather than a rebuild --
claude/shard-itest survives at e020534, and the branch list now says
explicitly not to delete it.

Flagged: the two levers that need no sharding -- the missing buildx layer cache
in itest/e2e, and the self-imposed 3s lockWaitTime in our versitygw fork -- were
born on the closed branch and exist nowhere on main. They are the live work now.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
Peeja pushed a commit that referenced this pull request Sep 17, 2026
Petra merged #20, #17, #22, #19, #23 and #24; main is 144b316 and the open
list is #25 and #26. Dropped the merged rows from both tables rather than
letting them accumulate.

#25's e2e red turned out to be the pre-existing filesystem flake, and the one
re-run that cleared it was warm -- so it produced the measurement as well as the
green: 8 images in 23s against a 7m34s baseline, and the e2e job from 13m10s to
5m29s. #26 puts that in MONOREPO_TODO.md with the caveats attached rather than
the headline alone.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
Peeja pushed a commit that referenced this pull request Sep 17, 2026
The previous entry claimed a naive subtree pull could merge upstream's unpinned
MinIO strings over our pinned ones without a signal, and told whoever does the
final pull to diff the pinned-image set as a mitigation. Petra challenged it and
it does not survive testing.

git merge-file on the real base/ours/theirs for each file: piri's minio.go
conflicts once, sprue's s3.go once, smelt's common/compose.yml twice. All three
conflict loudly. In each, the digest-bearing line survives the merge -- the
conflict is confined to the minio.Run(...) call, while the const carrying the
digest sits where upstream did not touch and merges cleanly.

Resolving to theirs was simulated too: it compiles and vets clean, but
staticcheck reports U1000 on both the orphaned const and the orphaned helper,
and ci.yml runs staticcheck on every module. The compose case is additionally
covered by check-stack-images.sh.

The useful part is why. The named-const-plus-helper shape #19 introduced makes
dropping the pin orphan dead code, so the indirection is self-guarding under
merge where a bare inline literal would not be. The deliberately-unguarded Go
image population was never exposed here.

The mitigation is removed rather than softened. A warning that is wrong in a
document people trust is worse than no warning.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants