test(tests): pin the keep annotation on cert-manager and etcd-operator CRDs - #3590
Conversation
📝 WalkthroughWalkthroughThe cert-manager and etcd-operator CRD charts now provide ChangesCRD Helm test coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
Reopening to pick up the multus fix on main. |
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
LGTM. The suites are honest — I mutated the tree to check, and every failure mode I could think of turns the run red rather than passing on an empty document set:
- dropping
keepfrom a single CRD (etcd-operator-crds/templates/etcdsnapshots.yaml) fails and names that template, so the asserts really do run per document rather than only against the first one; crds.keep: falseincert-manager-crds/values.yamlfails across all six CRDs;crds.enabled: false, where the chart renders nothing at all, fails withno manifest found— no vacuous pass;- moving the CRDs out of
templates/*.yamlentirely fails the same way; - moving one CRD into
templates/sub/and unstamping it still fails, so the glob reaches nested paths and a future refill into a subdirectory stays covered; - adding a non-CRD document fails on the
kindassert, which is the invariant the header comment describes.
No merge-order dependency on #3586: that PR covers four different packages and ships its own suites for them, this one covers the two that already carry the annotation on main. The suites pass on this branch's base with #3586 absent. There is a textual overlap with #3595 in etcd-operator-crds/Makefile, but in a different hunk — whichever lands second may want a trivial rebase, and the two are complementary rather than redundant: #3595 fails the vendoring, this suite guards the committed files.
The red Unit & controller tests check is unrelated to this diff. It fails at the bats step on the EXIT-trap freeze list (multus-install-cni-plugins.bats=12 is not in the frozen set), which #3584 fixed on main after this branch's base. In that same run the helm suites ran and passed — the log has Running tests in packages/system/cert-manager-crds, the same for etcd-operator-crds, and All Helm unit tests passed. A rebase should clear it.
Two nits, neither blocking:
- This suite and the ones in #3586 share a filename and a purpose but not a shape: there it is
documentSelectorwithmatchMany: true, named per-CRD cases and ahasDocumentscount; here it is a template glob with anequalonkind. The divergence is defensible — the gateway-api chart genuinely renders non-CRD documents, these two do not — but if both land the tree carries two idioms for one job. Worth a line in one of them saying why, or converging on one. - The
kindassert encodes "this chart renders only CRDs" as a second invariant. That is deliberate and the comment says so; just note that when it trips, the failure reads as akindmismatch rather than pointing at the annotation.
One observation beyond this diff, since it is the same argument this PR makes about the test: target being load-bearing. Four packages have tests/ fixtures and no test: target, so hack/helm-unit-tests.sh has never executed them: packages/apps/mongodb (7 suites, 94 tests), packages/system/gpu-operator (3 tests) and packages/system/monitoring-agents (2 tests) — all three pass locally today — plus packages/system/nfs-driver, whose tests/ holds pvc.yaml/sc.yaml rather than helm-unittest fixtures. Separately, packages/system/piraeus-operator-crds (4 CRDs) and packages/system/application-definition-crd (1 CRD) render CRDs from templates/ with neither the annotation nor a suite; they are not tenant addons, so outside #3586's stated scope, but the same uninstall cascade applies. Both look like follow-ups rather than anything for this PR.
The CRDs carry helm.sh/resource-policy: keep, which is what stops a release uninstall from deleting them and taking every Certificate, Issuer and Order in the cluster with it. Nothing asserted that. The annotation is not local to this package on either side: it comes from the upstream template under a conditional, the value that switches it on is ours, and the templates are refilled from the vendored upstream chart by packages/system/cert-manager on its own update. The assertion reads the rendered output, so it does not depend on which of those moves. Scope by wildcard rather than a list of filenames, so a CRD that upstream starts shipping is covered without editing the test, and assert the kind alongside the annotation, since the annotation assertion on its own accepts a non-CRD document that carries it. Add the test: target too. hack/helm-unit-tests.sh runs a package's suite only when its Makefile defines that target, so without it the fixtures sit unread and the run reports success on a chart that has already lost the annotation. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
The CRDs carry helm.sh/resource-policy: keep, which is what stops a release uninstall from deleting them and taking every EtcdCluster, EtcdMember and EtcdSnapshot in the cluster with it. Nothing asserted that. The annotation is stamped by the awk step in `make update`, anchored on the controller-gen version line the upstream CRDs carry. If that line stops being emitted, the step matches nothing and exits zero. Scope by wildcard rather than a list of filenames, so a CRD added to the vendoring list is covered without editing the test, and assert the kind alongside the annotation, since the annotation assertion on its own accepts a non-CRD document that carries it. Add the test: target too. hack/helm-unit-tests.sh runs a package's suite only when its Makefile defines that target, so without it the fixtures sit unread and the run reports success on a chart that has already lost the annotation. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
43157cd to
ac2f882
Compare
## What this PR does `make update` in `packages/system/etcd-operator-crds` vendors three CRDs and stamps `helm.sh/resource-policy: keep` onto each with an awk step anchored on the `controller-gen.kubebuilder.io/version:` line. That line is upstream's to emit. If a future controller-gen stops writing it, the awk matches nothing, writes the CRD out unstamped and exits zero, so the regeneration reports success and the CRDs land without the annotation that keeps a chart uninstall from deleting them, and with them every EtcdCluster, EtcdMember and EtcdSnapshot in the cluster. The step now counts what it produced and requires exactly one annotation per CRD. Zero means the anchor is gone; more than one means it matched somewhere unintended. Every CRD is staged in a temporary directory and `templates/` is replaced only after all of them have been counted, so a failure on any one leaves the vendored files exactly as they were rather than wiping the directory and refilling part of the set. I checked it by substituting the upstream response, so what ran was the recipe that ships rather than a copy of its logic: a CRD with no controller-gen line fails with `stamped 0 keep annotations, expected 1`, a CRD carrying that line twice fails with 2, a failure on the second of the three leaves all three committed files untouched, and the real upstream still vendors byte-identical output, with `make update` leaving `git status` showing nothing but this Makefile. On `main` today this is the only thing guarding those three CRDs. A helm-unittest suite that fails when one of them renders without the annotation is in #3590, which is open and not merged. Once that lands the two catch the same loss at different moments: the suite on a test run, this step in front of whoever ran the regeneration. The same stamping shape exists in three more packages, but only on the branch of #3586, which adds it to `gateway-api-crds`, `vsnap-crd` and `vertical-pod-autoscaler-crds`. There is nothing to guard there on `main` yet, so they are deliberately out of this change and need the same step once #3586 merges. ### Screenshots Not a UI change. ### Downstream repositories - [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: I walked the trigger map in `docs/agents/contributing.md` against the file list of this diff, which is one package Makefile and nothing else. `hack/package.mk` is untouched, so the `ccp` and `external-apps-example` triggers that key on it do not fire, and the website's "developer tooling (the package Makefiles)" line is the only near miss: `content/en/docs/next/development.md` describes `make update` in general terms and does not go stale from a package validating its own output. ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved CRD update validation to ensure generated definitions include the required preservation annotation. * Prevented incomplete or invalid updates from replacing existing CRD templates. * Added automatic cleanup of temporary update files. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
cert-manager-crdsandetcd-operator-crdsare the two packages this repository points at as the in-tree precedent for protecting CRDs withhelm.sh/resource-policy: keep. Both carry the annotation, neither had a test, so it could be lost with nothing reporting the loss. This adds a suite to each, plus thetest:target thathack/helm-unit-tests.shneeds before it will run one. No annotation is added and no rendered output changes: the point is to close the way of losing what is already there.The two are exposed differently, so the suites say so instead of sharing one explanation. In
etcd-operator-crdsthe annotation is ours.make updatestamps it with an awk step anchored on thecontroller-gen.kubebuilder.io/version:line that the upstream CRDs carry. If a future controller-gen stops emitting that line, the step matches nothing, writes the CRDs out unstamped and exits zero.In
cert-manager-crdsthe annotation comes from the upstream template, emitted under{{- if .Values.crds.keep }}, and the value that switches it on is ours. That is two independent ways to lose it. The templates are also not maintained in that package:packages/system/cert-manager/Makefiledeletes and refills itstemplates/directory during its ownmake update, so a cert-manager bump rewrites a neighbouring package that whoever runs the bump has no reason to open.Each suite is scoped with the wildcard
templates/*.yamland asserts across every document rendered under it, rather than listing CRD names or pinning a document count, so a CRD that upstream starts shipping is covered without editing the test. Each also asserts the kind, because the annotation assertion on its own would accept a non-CRD document that happened to carry it. Both charts render only CRDs today, checked withhelm template.The
test:target is load-bearing rather than boilerplate.hack/helm-unit-tests.shruns a package's suite only when its Makefile defines that target. With the target present, breaking a chart makes the whole run exit 1 and name the directory. With the fixtures present but the target removed, the same broken chart gives exit 0 and "All Helm unit tests passed", having executed the suite zero times.Screenshots
Not a UI change.
Downstream repositories
I walked the trigger map in
docs/agents/contributing.mdagainst the file list of this diff. It adds two test files and twotest:targets underpackages/system/, and touches nothing else: nohack/package.mk, no appvalues.schema.json, no platform values, noApplicationDefinition, no release asset names. The website's "developer tooling (the package Makefiles)" trigger is the one worth checking, so I readcontent/en/docs/next/development.mdthere: it documentsmake testin general terms and does not go stale from a package gaining that target.Release note
Summary by CodeRabbit
Tests
keepresource policy during Helm operations.Chores