test(e2e): pin the release-history read against the upstream type - #3607
test(e2e): pin the release-history read against the upstream type#3607Aleksei Sviridkin (lexfrei) wants to merge 2 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe remediation guard now exports a shared HelmRelease history JSONPath. Kubernetes tests use it instead of a local literal. New contract tests validate extraction, Flux serialization, status ordering, assignment visibility, and call-site wiring. ChangesFlux HelmRelease history contract
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
45c1da7 to
8bac54d
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hack/e2e-chainsaw/_lib/run-kubernetes.sh (1)
1470-1481: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftEnforce the diagnostic deadline inside these collectors.
cozy_diag_phase_has_timechecks only before a collector starts. It cannot interrupttalos_image_cache_diagnose, which contains unbounded calls.ghcr_mirror_diagnosealso falls back to unboundedkubectlcalls whentimeoutis unavailable. A wedged API can preventcozy_report_node_join_failurefrom reaching theexit 1that triggers_tenant_snapshot_on_fail, so the failure snapshot can be lost. Pass the remaining deadline into these helpers and enforce it for every external call, or make the timeout dependency mandatory.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/e2e-chainsaw/_lib/run-kubernetes.sh` around lines 1470 - 1481, Update ghcr_mirror_diagnose and talos_image_cache_diagnose to receive the remaining diagnostic deadline and enforce it around every external command, including kubectl and management-cluster calls; alternatively, make the timeout utility mandatory and fail safely when unavailable. Ensure a wedged API cannot block cozy_report_node_join_failure from reaching its failure snapshot and exit path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@hack/e2e-chainsaw/_lib/run-kubernetes.sh`:
- Around line 1470-1481: Update ghcr_mirror_diagnose and
talos_image_cache_diagnose to receive the remaining diagnostic deadline and
enforce it around every external command, including kubectl and
management-cluster calls; alternatively, make the timeout utility mandatory and
fail safely when unavailable. Ensure a wedged API cannot block
cozy_report_node_join_failure from reaching its failure snapshot and exit path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e4f1ee6-1a76-44e0-a75d-974a8597b046
📒 Files selected for processing (1)
hack/e2e-chainsaw/_lib/run-kubernetes.sh
IvanHunters
left a comment
There was a problem hiding this comment.
Overview
This PR deletes the status.history extraction pins HR v2 status.history shape test in hack/remediation-guard.bats, which wrote a HelmRelease fixture containing the field names it expected and then read that same fixture back with the same jsonpath — proving only that yq can read a document the test itself composed, not that the shape matches the real upstream type. It replaces this with a Go contract test (internal/fluxcontract/snapshot_shape_test.go) that reads the jsonpath expression directly out of the shipping script (hack/e2e-chainsaw/_lib/run-kubernetes.sh) and runs it against a HelmRelease built from the actual github.com/fluxcd/helm-controller/api/v2 types.
Verification performed
I did not take the PR's claims at face value; I cloned the branch and reproduced each one:
- Upstream type check:
go.modon this branch pinsgithub.com/fluxcd/helm-controller/api v1.5.1. Fetchedapi/v2/helmrelease_types.goandapi/v2/snapshot_types.gofrom theapi/v1.5.1tag directly fromfluxcd/helm-controller— confirmedHelmReleaseStatus.HistoryisSnapshots(json:"history,omitempty") andSnapshot.Statusisstring(json:"status"). The test's use ofgithub.com/fluxcd/helm-controller/api/v2(a package path, not a go-modules major-version suffix) is correct for this module layout. - Sole occurrence check: grepped the full script for
jsonpath—.status.history[*]appears exactly once, athack/e2e-chainsaw/_lib/run-kubernetes.sh:2289, matching the PR's claim that only one read needs pinning today. - Regex correctness: hand-traced
historyExpragainst both quoting styles used elsewhere in the script (-ojsonpath='...'and-o 'jsonpath=...') — both are matched correctly, and the literal\{range \.status\.history\[\*\]\}anchor prevents it from picking up unrelated jsonpath expressions in the same file (there are ~20 others). - Mutation testing (reproduced, not just read):
- Renamed
.statusto.statuszin the shipped jsonpath expression -> test failed withstatusz is not found, exactly as described. - Replaced the history read with an unrelated jsonpath (
{.status.phase}) -> test failed with the "guard asserts nothing" message, exactly as described. - Restored the file, ran
go test ./internal/fluxcontract/...clean -> passes.
- Renamed
- Build hygiene:
go build ./internal/...,go vet ./internal/fluxcontract/..., andgofmt -lon the new file are all clean. - CI wiring:
.github/workflows/pull-requests.yaml:213-214runsmake test-controllers, which isgo test ./internal/... -count=1in theMakefile— the new package is picked up with no registration needed. - Coverage after deletion: the remaining
hack/remediation-guard.batshas 7 tests (confirmed by count), includinguninstalled then deployed still returns detected, which still covers thehelmrelease_has_remediation_cyclehalf that the deleted test also exercised. No dangling references to the removed test remain anywhere in the tree. - Diff scope vs. CodeRabbit's outside-diff comment: CodeRabbit flagged a pre-existing timeout-enforcement gap in
ghcr_mirror_diagnose/talos_image_cache_diagnosearound lines 1470-1481. Confirmed viagit diffhunk headers that this PR's only hunk in that file is@@ -2280,8 +2280,11 @@— the flagged code is untouched by this change and is a pre-existing concern unrelated to this PR's scope, not a regression introduced here.
Verdict
No regressions, no unverifiable claims, and the brittleness question the PR is meant to answer checks out both ways: the new test correctly fails when the specific field the e2e script depends on is renamed (catching real breakage), and it does not depend on any upstream field it doesn't actually use, so it will not go red on unrelated upstream churn. This is a solid, betterment-only change.
Non-blocking notes
- Single-match scope, already disclosed:
readGuardExpressionusesregexp.FindSubmatch(first match only). The PR body explicitly calls this out as a known boundary — a second.status.historyread added later would go unpinned silently. Author has already scoped theFindAllSubmatchfollow-up to "when it starts to matter" (i.e., when a second read is added), which is a reasonable call given there is exactly one read today. No action needed now. - Nice use of mutation-based self-verification in the PR description — both directions (rename breaks it, dropped-read is caught with a clear message) are exactly reproducible, which is unusually strong evidence for a test-of-a-test change.
Approving.
myasnikovdaniil
left a comment
There was a problem hiding this comment.
Mutation checked the pin. {.status} to {.statusz}, swapping the history read for a conditions read, dropping the {"\n"} separator, all go red with the message you wrote. Simulated a renamed history json tag and it fails under both AllowMissingKeys settings, so an upstream schema change won't come back silently empty.
One finding inline that is not in your blind spots list.
Small thing, comment at the validator says "the way kubectl runs it" but jsonpath.New defaults to AllowMissingKeys(false) and kubectl runs with true. Divergence is in the safe direction, only the comment is off.
Also internal/ is in full_suite_pattern (hack/select-e2e.sh:85), so this test-only file escalates the PR to the full chainsaw suite. Just so the cost is known.
| if err != nil { | ||
| t.Fatalf("reading %s: %v", guardScript, err) | ||
| } | ||
| m := historyExpr.FindSubmatch(src) |
There was a problem hiding this comment.
Regex is not anchored to a command line, so a jsonpath sitting in a comment satisfies the pin and hides a broken real read. I pasted the correct expression into a comment above a read mutated to {.statusz} and the test stayed green.
This is not theoretical here, the PR adds a comment block at run-kubernetes.sh:2280-2285 right above that command discussing the expression, which is exactly where someone pastes an example later.
FindAllSubmatch plus asserting every match, with a # guard or a kubectl anchor, closes this and the only-first-read gap you already noted.
There was a problem hiding this comment.
You were right about the version you reviewed, and I reproduced it before touching anything. At 8bac54d the test scraped the expression out of run-kubernetes.sh with FindSubmatch, so with the correct expression sitting in a comment above a read carrying {.statusz}, the first match is the comment and the test then validates the comment against the upstream type.
That path is gone. The expression is no longer read out of the script at all: it lives in a named assignment in remediation-guard.sh, and the test sources that library and takes the value the shell is left holding. Nothing written in a comment can supply the expression under test any more.
The caller side still checks text, so I measured it rather than trusting the anchor. Your three comment shapes are refused today: the pattern starts at ^[ \t]* followed by the local's own name, and a # cannot begin that. What was not refused is the same class in a shape a comment cannot reach: the check asked whether a well formed read existed anywhere in the file, not whether the read the guard actually runs is that one.
So the pin now starts at the call rather than at the text. It takes the local handed to helmrelease_has_remediation_cycle, requires that local to appear followed by = exactly once in the file, and requires the read filling it to take the shared expansion. The count is of occurrences rather than of parsed assignments, so it errs toward one too many, which reddens, instead of one too few, which would not. Same mutations against the old and the new test, unmutated control green in both:
| mutation | before | after |
|---|---|---|
read rewritten to {.statusz} |
red | red |
| correct expression pasted in a comment above it | red | red |
| plus a second assignment to the same local | green | red |
| plus a correct read filling another local | green | red |
| guard handed a different local | green | red |
| guard call removed | green | red |
| local renamed on both sides | green | green |
| shared expression broken in the library | red | red |
Each of the three assertions carries its own accept and refuse cases, so a spelling outside them reddens with the pattern printed instead of passing quietly.
f4a9d52 to
8821c6d
Compare
myasnikovdaniil
left a comment
There was a problem hiding this comment.
The pin was verified against an actual upstream rename rather than against a fixture, and the bats test it deletes was proven vacuous.
The version question resolves cleanly. go.mod:11 pins github.com/fluxcd/helm-controller/api v1.5.1, the test imports api/v2 from that module and builds the fixture from the upstream Go struct instead of transcribing a shape, so it cannot drift from the vendored version by construction. A bump that renames the field is a red test in the same go get, and a bump that does not is invisible to it, which is the right split.
The renames were performed for real, by copying helm-controller/api@v1.5.1 out of the module cache, editing the json tags and adding a replace. json:"history" to "releaseHistory" gives history is not found. The serialized shape no longer satisfies the expression. Same for Snapshot.Status to phase. Both leave the deleted bats test green.
That last part settles the deletion. The old test named the upstream type only in a comment and asserted with yq over a heredoc it wrote itself, so it read neither side of the contract. Repointing the shell library's expression at .status.snapshots leaves it green and prints Test OK: status.history extraction pins HR v2 status.history shape. It could not fail for the reason its name gave, so deleting rather than repairing is right, and its useful coverage survives elsewhere: the integration leg in the uninstalled then deployed test, the output-format leg more strictly in the Go compare.
I checked kubectl's printer semantics against client-go/util/jsonpath too, and both justifications in the file hold. With --allow-missing-template-keys at its default a renamed-away path gives empty output and exit 0, while a braceless template is echoed back verbatim and non-empty. That is exactly why pinning the $ matters and pinning the quoting does not.
Three things.
The changed read has never executed in CI on this PR. run-kubernetes.sh:2285 sits after the node-join gate at :1298, both suites died there, and grep -c "Parent HelmRelease history statuses" over the full log returns 0. I closed it by hand instead of leaving it open, traced through a fake kubectl on PATH under bash -eu: the shipped form produces the single argument -ojsonpath={range .status.history[*]}{.status}{"\n"}{end}, byte-identical to the base inline literal, and the chain into helmrelease_has_remediation_cycle correctly reports a cycle. Removing the source line aborts loudly on unbound variable. No defect, but record it as verified by hand rather than by CI.
The pin covers the path and serialization, not the vocabulary. A bump that records install remediation as uninstalling rather than uninstalled leaves the path resolving, so the read returns non-empty, the emptiness backstop stays quiet, and grep -E '^(failed|uninstalled)$' matches nothing while a real remediation race passes. Not cheaply fixable, the snapshot status constants are unexported and helm.sh/helm is not a dependency, so I am not asking for it. A sentence in the docstring saying the pin is on the path and not the values would stop the next reader over-trusting it.
And docs/agents/e2e-testing.md:65 says a failed or uninstalled Snapshot survives a later successful reconcile. Snapshots.Truncate removes everything up to the previous deployed-or-superseded snapshot, so it survives one later deploy and not two. Pre-existing, but this PR is rewriting that exact line, which is the moment to qualify it.
The red pre-commit is infrastructure, the cozyvalues-gen download returned non-gzip content and the step died before any hook ran. Re-run it.
8821c6d to
04505f6
Compare
9007eb7 to
1a9ddd9
Compare
1d3ddfb to
68afacd
Compare
8cc66be to
0aa43e1
Compare
The test that claimed to pin the HelmRelease v2 history shape wrote a HelmRelease document containing the field names it expected, then read that same document back with the same jsonpath. It established that yq can read a fixture the test itself composed. Its own comment described the failure it was meant to catch: a future Flux release renames the field, the jsonpath matches nothing, the guard reports no remediation cycle, and real cycles pass the e2e assertion. A rename moves neither the fixture nor the expression, so the test stays green through exactly that event. The comment also named where the truth lives, github.com/fluxcd/helm-controller/api/v2, and pinned a copy of the author's expectations instead. Replace it with a Go test that supplies neither half. The expression is read out of the shell library that ships it, and the object is built from the upstream type at the version go.mod holds, then serialized and matched the way kubectl does. A renamed or retyped field is a compile error or a failed match; a guard that stops reading history fails the lookup with a message saying so. Both directions were confirmed by mutation. Locating the expression by searching the file is not enough on its own. Inline in a long script, locating it means deciding which occurrences are code and which are prose, and every way of getting that wrong fails green: an example quoted near a broken read satisfies the pin while the guard reads nothing. So the expression moves into a named assignment in remediation-guard.sh, which the guard already sources, and the test sources that library and takes the value the shell is left holding. Deciding which text is the assignment is then not the test's problem: an `export`, an indent, a second assignment later in the file all resolve the way the guard will resolve them, because the thing resolving them is the shell. Sourcing borrows the environment along with the shell, so the snippet unsets the name before it sources. `go test` hands the child its own environment, and a shell that had exported this name would answer in place of a library that stopped assigning it: the value comes back non-empty, the empty check stays quiet, and the pin passes over a guard that reads nothing. That is this file's own subject reaching it through the environment rather than through the source. Moving the expression out of the call site puts a step between what is pinned and what runs, so the call site is checked too, and the check starts from the call rather than from the text: the local the guard is handed must be assigned exactly once, by an anchored assignment whose jsonpath is the expansion of the shared name. That a well-formed read exists somewhere in the file is a weaker claim than it reads as, since a second assignment to the same local, or a read filling another one, satisfies it while the guard runs a literal nothing here pins. The expansion and not the name: kubectl echoes a braceless template back verbatim, so a read that dropped the `$` would return its own text, clear the script's empty-history backstop, and carry no status the cycle check matches. The library is checked by sourcing rather than by reading, so an assignment the shell would run is resolved by the shell instead of by a guess about which text the shell would pick. Each rule is pinned by its own test, so a regression names which one it opened rather than only that extraction broke. How kubectl's printer treats an absent path is its own configuration rather than part of this contract, so it is left unasserted: guessing at it would put the expectation and the fixture back in the same hands. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
0aa43e1 to
fa588c8
Compare
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM.
Test/docs only. Proved non-vacuity by mutation: breaking the jsonpath {.status} to {.statusZZ} and inlining the expression instead of the shared var each turn a test red. The read is pinned against the versioned public type fluxcd/helm-controller/api/v2.Snapshot, not an ad-hoc parse, and CI actually runs it via make test-controllers. No coverage lost.
- [NIT] internal/fluxcontract/snapshot_shape_test.go is comment-heavy (roughly half narrative); consider trimming to the load-bearing "why".
What this PR does
hack/remediation-guard.batscarried a test called "status.history extraction pins HR v2 status.history shape". It wrote a HelmRelease document containing the field names it expected, then read that same document back with the same jsonpath throughyq. What it established is thatyqcan read a fixture the test itself composed.Its own comment named the failure it was there to catch: a future Flux release renames the field, the jsonpath matches nothing, the guard reports no remediation cycle, and real cycles pass the e2e assertion. A rename moves neither the fixture nor the expression, so the test stays green through exactly that event. The comment also named where the truth lives,
github.com/fluxcd/helm-controller/api/v2, and then pinned a copy of the author's expectations instead.internal/fluxcontractsupplies neither half. The expression is read out ofhack/e2e-chainsaw/_lib/remediation-guard.shat test time, so it is the one that ships rather than a copy. The object is ahelmv2.HelmReleasebuilt from the upstream type at the versiongo.modholds, serialized and matched throughk8s.io/client-go/util/jsonpath, which is the engine kubectl's-o jsonpathuses. A renamed or retyped field is then a compile error or a failed match, and a guard that stops reading history fails the lookup with a message saying so.That is one test. Two more exist so that it reads the expression kubectl actually receives, because the expression now lives in a named assignment the test sources and the caller expands: between the value checked here and the value that runs there is a shell variable, and each step of that indirection can fail while the suite stays green. The first follows the caller's own call: it reads the local handed to
helmrelease_has_remediation_cycle, requires that local to be assigned exactly once, and requires that assignment to take the expansion of the shared name rather than the bare name, which matters because kubectl echoes a brace-less template back verbatim, so a read that lost its$returns its own text, clears the script's empty-history check, and matches no status. Starting from the call rather than from the text is what makes the pin describe the read that runs: asking only whether a well formed read exists somewhere in the file is satisfied by a second assignment overwriting the same local, and by a read filling a local the guard never consumes. The second holds that no shell library but the one that owns the name assigns it, which does not follow from the first: an assignment beside the read satisfies the first and still replaces the value after this file has approved the one the library exported. It covers every library in that directory rather than the caller alone, because the caller sources three of them and the owning one first, so an assignment in any of the others lands later and wins.Those two match text with a pattern, and each carries both halves of its contract. A refuse table lists the forms the pattern must reject, and an accept table lists the forms it must keep accepting, one case per freedom the code claims: for the call, the
ifprefix, the negation, an expansion with or without braces; for the read, the spelling of-o, the quoting of the substitution, an expansion with or without braces, and the local's name, which stays free because the call and the read are renamed together. Without the accept side those freedoms survive only until someone tightens the pattern, and nothing reddens when they go.Two boundaries, named rather than left to be found. The pin holds the shape of the read, not the vocabulary of the answer: the guard greps for
failedanduninstalled, and the test supplies those two strings itself, so a release status renamed upstream leaves the jsonpath returning strings, the test green because it wrote them, and the shell grep silently matching nothing. Those values come from Helm's release status rather than the Flux type, so pinning them reaches past what this change binds, and there is more than one way to do it. Second, a literal history read that fills a local the guard never consumes is not caught, and on its own that is harmless: the guard still runs the shared expression, and the moment it stops doing so the read filling the consumed local no longer matches and the caller test fails.The caller-side half is a heuristic and is meant to be read as one. It decides whether a piece of text is the read by matching it, and a matcher that reasons about shell is wrong at some spelling by construction, which is the same objection the file raises against reading the library's assignment as text instead of sourcing it. Sourcing is available for the library and not for the caller, so what is left is to make the imprecision run one way: the count is of occurrences, so an extra one reddens and a missed one cannot pass, and a form outside the accepted set fails with the pattern printed rather than quietly stopping asserting.
That direction is the whole argument for keeping it. A false red is a broken build on a legitimate rewrite, and the failure names what it did not recognise, so it costs a minute. A silent green is a guard that asserts nothing for as long as nobody looks, which is the state this branch exists to end. The ways the caller can break without anyone noticing are worth that trade. A read that lost its
$hands kubectl a template with no braces, which comes back as the variable's own name; a hand-written path that is valid but names the wrong field comes back as that field's values. Both are non-empty, so the script's own empty-history backstop stays quiet and the cycle grep matches nothing. A deleted call is quieter still, since there is then no assertion to fail. None of the three is visible to the library-side pin, which judges the value the library hands out and never learns whether it reached kubectl. What does fail loudly is an expression kubectl cannot use at all, and that is the one case needing no pin.The shape the fixed test now has already exists in this tree, in
hack/seaweedfs-naming-audit.bats, under "the audit's reconstruction agrees with the chart helper it mirrors". That one also writes a heredoc, which is what makes the pair worth comparing: it copies the real_naming.tplout of the chart, renders it throughhelm, and checks the audit script's output against the render. The heredoc is a harness for calling the real helper, not a record of what the author expected it to say, and the two sides have different authors. A test that writes both sides proves only that it is self-consistent.Nothing is lost by the deletion. The removed test's tail fed extracted statuses into
helmrelease_has_remediation_cycle, and the seven tests remaining in the bats filehack/remediation-guard.batsstill cover that, includinguninstalled then deployed still returns detected. The extraction half is now pinned more strictly than before: the old test asserted only that the output was non-empty and contained one line, the new one asserts the exact two bare tokens, so dropping the{"\n"}separator also goes red.CI runs it.
make test-controllersis its own step in.github/workflows/pull-requests.yamland runsgo test ./internal/... -count=1, which picks the new package up without registration.Screenshots
Not a UI change.
Downstream repositories
Walked the trigger map against the diff. It adds a Go test package under
internal/, deletes one test fromhack/remediation-guard.bats, adds a variable tohack/e2e-chainsaw/_lib/remediation-guard.sh, points the kubectl invocation inhack/e2e-chainsaw/_lib/run-kubernetes.shat that variable, adds a sentence todocs/agents/e2e-testing.md, and recordsmake test-controllersindocs/agents/overview.md. No package is added, renamed or removed, and nothing touches avalues.schema.json, a version enum, avalues.yamldefault, anApplicationDefinition, a CRD, a namespace, an annotation or a telemetry metric.hack/e2e-prepare-cluster.batsis untouched, so the node contract thattalmandansible-cozystackrestate by hand does not move.The closest call is
ccp, whose skills anchor on files underhack/. Its trigger is a move, a rename, or a make target that does something different. This changes the contents of three files, not their names, and no target behaves differently;hack/package.mkandhack/common-envs.mkare untouched.Release note