refactor(e2e): make the fixture minimal and take the images from it

[vyncint]

Takes the suggestion from #2306: minimal rather than defaulted, and extract only what does not vary.

Inverted

The defaulted builders are gone — WithFluentbit, WithFluentd, FluentdSpec, Workers, Drain, Buffer, Timekey, TimekeyWait, HTTPOutput, Flow. None had a caller, and both suites that could have used one declined it because its defaults inject fields they do not have.

Two measurements settled the shape rather than assumption. Only ControlNamespace appears in all 17 Logging literals — even EnableRecreateWorkloadOnImmutableFieldChange is 12 of 17, so the old builder was already injecting a divergence into a third of its callers.

Logging itself is gone too. A seven line minimal constructor with no caller invites the next person to guess at its intent, and whichever migration wants one can add it back. Nothing exported here is now without a caller: ReceiverURL and TenantReceiverURL are both used by tenancy.go.

tenancy.go stays. It is a whole-scenario constructor rather than a defaulted builder over divergent fields, it has two callers, and TestBuildersMatchCommon pins it equal to common.

Extracted

99 image literals across the module split 91 v1beta1.ImageSpec and 8 v1beta1.BasicImageSpec in three suites, which is why Basic is a converter rather than a second name per image. Nothing overrides an image: there is not one hardcoded Repository: or Tag: anywhere in the suites.

81 of those literals are swept here, across eight suites. TestImageHelpersMatchTheLiterals pins each helper against the literal it replaces, so the sweep cannot change which image a suite runs. The four suites with changes in flight are left alone to avoid a conflict; they pick the helpers up with their own.

The constants themselves are duplicated from common, with TestImageNamesMatchCommon holding the two equal. That is unavoidable while the unmigrated suites reference common's copies, and this is the PR that makes 81 call sites depend on the fixture's, so the condition for removing both is now written beside them: when the last suite stops using common's, the duplicates and the test go and fixture is the only source.

One test earned its removal. TestTenancyAggregatorDivergesFromTheDefault compared the tenancy spec against FluentdSpec(); with no default to diverge from the comparison means nothing, so it asserts the spec's own properties and is named for what it does.

Not folded in

The two things flagged as separate stay separate: the e2e-framework helm binary that watch-selector needs and no Makefile installs, and whether the syslog-ng spec is the one case for testdata YAML.

Verification

make check passes, golangci-lint run --max-same-issues=0 --max-issues-per-linter=0 in e2e/ reports 0 issues, and both commits build, vet and test on their own. The sweep is mechanical and its equivalence is asserted rather than assumed; no suite behaviour changes, so there is nothing here a cluster run would exercise that the unit tests do not.

[csatib02]

This is the shape I was arguing for, and the two measurements are what make it convincing rather than a matter of taste. ControlNamespace in 17 of 17 and EnableRecreateWorkloadOnImmutableFieldChange in 12 of 17 settles it: the old builder was injecting a divergence into five of its would-be callers, which is exactly the failure the defaults were supposed to prevent. Worth having measured before deleting rather than after.

TestImageHelpersMatchTheLiterals is the right guard for an 81-literal sweep — it makes the mechanical part assertable instead of reviewable by eye, and I agree there is nothing for a cluster to prove that it does not.

Keeping tenancy.go is right for the reason you give: it is a whole-scenario constructor with two callers, not a defaulted builder over divergent fields. Different thing that happened to live in the same package. And removing TestTenancyAggregatorDivergesFromTheDefault rather than updating it is correct — a comparison against a default that no longer exists cannot fail for a real reason.

Two things, neither blocking.

Logging and Flow still have no callers. After the inversion they are 7 and 1 lines rather than 84 and 61, so this is a much smaller version of the same thing — but nothing in the module calls either, so the new shape is argued rather than exercised. Everything that got adopted here is images and tenancy. I would rather delete them and let batch 3 add back whatever it actually wants, since a 7-line minimal constructor is trivially re-addable and a callerless one invites the next person to guess at its intent. If you would rather keep them as the seed for the next migration, worth saying so in the code, because otherwise they read as leftovers.

The image constants are duplicated, and a test is holding the two copies equal. internal/fixture/images.go:21-25 declares FluentdImageRepo and friends as its own string literals, and fixture_test.go pins them against common.FluentdImageRepo. So the module carries two copies of every image constant with a test as the sync mechanism.

That predates this PR — the constants arrived with internal/fixture in #2299 — but this is the PR that makes 81 call sites depend on the fixture copy, so it is the moment the duplication stops being incidental. It is genuinely unavoidable while the four in-flight suites still reference common.*, so I am not asking for it now. What I would ask for is the expiry stated the way you stated the equivalence test's: when the last suite stops using common's copies, both those constants and this test go, and fixture becomes the single source. Otherwise a transitional duplicate with a green test protecting it is the kind of thing that is still there in a year.

Ordering, since all three of your open PRs are disjoint: this one is worth landing before the next migration batch, so batch 3 is written against the final shape rather than reworked into it. It does not need to wait for #2306, which shares no files with it.

[csatib02]

We converged on this by measurement rather than up front, so it is worth writing the rule down now that three PRs have tested it.

The rule: extract a value when it is identical across all callers and no test asserts it. Otherwise it stays in the suite.

Checked against every candidate in this module:

candidate	identical	asserted	verdict
image names	99/99	no	extract
receiver URL	12/12	no	extract
Resources	6 of 10	no	leave
EnableRecreateWorkloadOnImmutableFieldChange	12 of 17	no	leave
Workers: 2	3 of 10	yes, it is what multiworker tests	leave

That predicts every outcome we reached the slow way, including the two builders you declined in #2306 and the ones you measured out here. It also explains why the defaulted builders failed: they extracted on resemblance rather than identity, so a default landed in callers that diverged. Worth keeping as the admission test for anything the next batches want to extract.

Two things the rule flags that are still open.

fluentd-full is declared in three places: common/helpers.go, common/setup/loggingoperator.go as defaultImages, and now internal/fixture/images.go. The one in setup is the list actually loaded into the cluster, so it is the only authoritative copy — the others agree with it by hand. Images are environment config rather than fixtures: they come from env vars the Makefile sets, and the loader already owns the list. Putting them on the harness alongside the loader makes it one declaration instead of three, and retires the dated duplicate note in this PR rather than waiting for the last suite to migrate.

ReceiverURL has no caller outside tenancy.go, while ten suites still write http://%s-test-receiver:8080/%s inline. By the rule that is the clearest extraction left in the module — identical everywhere, asserted by nobody — and it belongs on the harness next to MustReceive, since the harness already owns the receiver. env.Receiver.URL(tag) is what #2291 sketched, and it was right about this one.

Where that leaves the packages. harness owns lifecycle, waits, receiver and images. wait owns conditions. What remains of fixture is whole-scenario constructors — tenancy.go today — and is worth renaming for that, because fixture currently means image constants, a URL formatter and a tenancy scenario, which is common re-forming under a newer name. Admission for a scenario is two or more suites needing the same situation, not the same fields.

No CR builders. The Logging and Flow literals are each suite's specification, and the measurements say no field in them is universal enough to default safely.

This supersedes §4 of #2291, which sketched fixture.Logging(..., WithFluentbit(), WithFluentd(Workers(2))). Its harness half landed as described; the fixture half is the part measurement rejected. I am updating the issue so batch 3 is not written against it.