mirror of
https://github.com/cozystack/cozystack.git
synced 2026-10-30 00:27:04 +09:00
Aleksei Sviridkin
1071802e3f
## What this PR does
`hack/remediation-guard.bats` carried 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 through `yq`. What it established
is that `yq` can 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/fluxcontract` supplies neither half. The expression is read
out of `hack/e2e-chainsaw/_lib/remediation-guard.sh` at test time, so it
is the one that ships rather than a copy. The object is a
`helmv2.HelmRelease` built from the upstream type at the version
`go.mod` holds, serialized and matched through
`k8s.io/client-go/util/jsonpath`, which is the engine kubectl's `-o
jsonpath` uses. 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 `if` prefix, 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
`failed` and `uninstalled`, 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.tpl` out of the chart, renders it through `helm`, 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 file `hack/remediation-guard.bats` still cover
that, including `uninstalled 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-controllers` is its own step in
`.github/workflows/pull-requests.yaml` and runs `go 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 from `hack/remediation-guard.bats`, adds a
variable to `hack/e2e-chainsaw/_lib/remediation-guard.sh`, points the
kubectl invocation in `hack/e2e-chainsaw/_lib/run-kubernetes.sh` at that
variable, adds a sentence to `docs/agents/e2e-testing.md`, and records
`make test-controllers` in `docs/agents/overview.md`. No package is
added, renamed or removed, and nothing touches a `values.schema.json`, a
version enum, a `values.yaml` default, an `ApplicationDefinition`, a
CRD, a namespace, an annotation or a telemetry metric.
`hack/e2e-prepare-cluster.bats` is untouched, so the node contract that
`talm` and `ansible-cozystack` restate by hand does not move.
The closest call is `ccp`, whose skills anchor on files under `hack/`.
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.mk` and
`hack/common-envs.mk` are untouched.
- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:
### Release note
```release-note
NONE
```