test(tenant, harbor, seaweedfs, api): anchor guards that pass on a missing document - #3639
Merged
Merged
Conversation
Each test asserted only `notExists: spec.upgrade.force`, which passes on a template that renders no document at all. Renaming the value that gates one of these releases makes the template render nothing and leaves its test green: measured on etcd.yaml, where the guard stayed green while a sibling suite's `equal` was the only thing that noticed the release had vanished. Pin each HelmRelease name alongside the absence so the assertion has a document to be about. Cover computeplane.yaml. Seven templates in this chart emit a HelmRelease and the list held six; the module arrived after the suite was written. The invariant holds there today, so this closes an unguarded template rather than a live regression — which is the same gap that produced this suite, whose first pass missed gateway.yaml. Correct two claims in the header. The template restriction was said to be there because namespace.yaml's `lookup` errors under helm-unittest; it does not, the lookup returns empty offline and the template renders, which is what its own header says and what namespace_ancestor_labels_test.yaml relies on. The restriction stays, for the reason that actually applies: it keeps an unrelated template breaking out of this suite's result. helm-controller also does not deprecate upgrade.force. The CRD vendored at internal/fluxinstall/manifests/fluxcd.yaml, which is what the pinned Flux 2.8.x ships, describes it as a replacement strategy and carries no deprecation marker; `recreate` is the neighbouring field that does. Later upstream documentation adds that the field is ignored under server-side apply, and that sentence is not in the pinned CRD, so the comment says which is which. The reason to keep the flag off is client-side apply, where it replaces resources instead of patching them. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Both suites addressed their subject by document index. An index is a position, so a template that gains a document ahead of the one under test moves every later assertion onto a different object, and what happens then depends on the assertion: measured by inserting one document at the head of each template, the cleanup suite reddens outright, while two of the four jobservice tests keep passing because a `notExists` against an object that never had the path is satisfied for the wrong reason. Indices are also easy to get wrong by hand. helm-unittest numbers documents in template source order, while `helm template` prints them sorted by kind, so the rendered output is not the order being counted. Every document these suites assert on is unique by kind within its template, so each test now names its subject. A selector that matches nothing fails, including on the `notExists` tests, so the anchor holds in the direction that was silent before. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Each suite forbade a rule by naming it in full, and helm-unittest compares every field it is handed by value equality, so one respelling walks past. For seaweedfs that is not theoretical. Ungating the cnpg block and adding a verb to the rule (get,list,delete rather than get,delete) is the same grant and more, and the guard stayed green while a Client instance held delete on another app's Postgres cluster. Bound the Role by shape: a Client renders exactly two rules, both in the core group, and the cnpg grant is gated between them, so ungating it adds a third whatever its spelling. The rules themselves stay unpinned because the secret list is long and changes on its own schedule. For cozystack-api, add a count bound beside the existing spelling assert rather than in place of it. The two catch different things and neither covers the other: templates/rbac.yaml carries no template directives, so the list is pinned at exactly sixteen rules, while a grant swapped in for an existing one leaves the count where it was and only the named-rule assert sees it. The count is pinned from both sides — a dropped grant is the failure this suite is about, since the apiserver then returns forbidden and List silently drops the dropdown. Measured on all three: append, delete, and swap. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Aleksei Sviridkin (lexfrei)
requested review from
IvanHunters,
Andrei Kvapil (kvaps),
Timofei Larkin (lllamnyp),
myasnikovdaniil,
scooby87 and
Kirill Ilin (sircthulhu)
as code owners
August 8, 2026 00:23
Contributor
📝 WalkthroughWalkthroughChangesHelm test assertion hardening
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related issues
Possibly related PRs
Suggested labels: 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/system/cozystack-api/tests/rbac_test.yaml`:
- Around line 16-26: Update the explanatory comment in the RBAC test to refer
directly to the complementary Cilium test and sixteen-rule count test, replacing
the inaccurate phrase “last two tests.” Preserve the existing explanation of how
those two tests bound the role.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 85685a0d-653b-445f-999c-4efcc6759a17
📒 Files selected for processing (5)
packages/apps/harbor/tests/cleanup_hook_test.yamlpackages/apps/harbor/tests/jobservice_storageclass_test.yamlpackages/apps/tenant/tests/no_upgrade_force_test.yamlpackages/extra/seaweedfs/tests/cleanup_client_test.yamlpackages/system/cozystack-api/tests/rbac_test.yaml
Comment on lines
+16
to
+26
| # | ||
| # The last two tests bound the role from two directions, and neither covers the | ||
| # other. The count bound pins the list at exactly sixteen: templates/rbac.yaml | ||
| # carries no template directives, so anything appended makes rules[16] exist and | ||
| # anything dropped takes rules[15] away, whatever the rule is called. It says | ||
| # nothing about a grant swapped in for an existing one, because the count does | ||
| # not move under a swap. The spelling assert catches that case for the one rule | ||
| # it names, and misses every other wording of it, because helm-unittest compares | ||
| # each field it is handed by value equality and `any: true` only selects which | ||
| # fields are compared. Changing the rule count on purpose means moving both | ||
| # indices below, which is the review this role should get. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the test references in this comment.
“The last two tests” is inaccurate. The objectstorage.k8s.io test at Line 47 is between the Cilium test and the sixteen-rule test. Name the complementary tests directly.
Suggested wording
-# The last two tests bound the role from two directions, and neither covers the
+# The Cilium access and rule-count tests bound the role from two directions, and neither covers the📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # | |
| # The last two tests bound the role from two directions, and neither covers the | |
| # other. The count bound pins the list at exactly sixteen: templates/rbac.yaml | |
| # carries no template directives, so anything appended makes rules[16] exist and | |
| # anything dropped takes rules[15] away, whatever the rule is called. It says | |
| # nothing about a grant swapped in for an existing one, because the count does | |
| # not move under a swap. The spelling assert catches that case for the one rule | |
| # it names, and misses every other wording of it, because helm-unittest compares | |
| # each field it is handed by value equality and `any: true` only selects which | |
| # fields are compared. Changing the rule count on purpose means moving both | |
| # indices below, which is the review this role should get. | |
| # | |
| # The Cilium access and rule-count tests bound the role from two directions, and neither covers the | |
| # other. The count bound pins the list at exactly sixteen: templates/rbac.yaml | |
| # carries no template directives, so anything appended makes rules[16] exist and | |
| # anything dropped takes rules[15] away, whatever the rule is called. It says | |
| # nothing about a grant swapped in for an existing one, because the count does | |
| # not move under a swap. The spelling assert catches that case for the one rule | |
| # it names, and misses every other wording of it, because helm-unittest compares | |
| # each field it is handed by value equality and `any: true` only selects which | |
| # fields are compared. Changing the rule count on purpose means moving both | |
| # indices below, which is the review this role should get. |
🤖 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 `@packages/system/cozystack-api/tests/rbac_test.yaml` around lines 16 - 26,
Update the explanatory comment in the RBAC test to refer directly to the
complementary Cilium test and sixteen-rule count test, replacing the inaccurate
phrase “last two tests.” Preserve the existing explanation of how those two
tests bound the role.
This was referenced Aug 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Five helm-unittest suites asserted things that stayed green when the thing they guard disappeared. No chart or template logic changes here — only the tests, and each fix is measured in both directions.
tenant
no_upgrade_force_test.yaml. Every test assertednotExists: spec.upgrade.forceand nothing else. AnotExistspasses on a template that renders no document at all, so renaming the value that gates one of these releases leaves its test green while the release it guards is gone. Measured onetcd.yaml: with the gate renamed, the suite reported 6 passed and a sibling suite'sequalwas the only thing in the chart that noticed. Each test now pins the HelmRelease name alongside the absence.The same file also covers
computeplane.yamlnow. Seven templates in this chart render a HelmRelease and the list held six; the module landed after the suite was written. The invariant holds there today, so this closes an unguarded template rather than a live regression. That is the gap that produced this suite in the first place, when the first sweep missedgateway.yaml.Two rationales in that header were wrong and are corrected. The template list was said to be there because
namespace.yaml'slookuperrors under helm-unittest; it does not, it returns empty offline and the template renders, which is whatnamespace.yaml's own header says and whatnamespace_ancestor_labels_test.yamlrelies on. Removing the whole list leaves the chart's suites passing, so the list stays for the reason that actually applies. And helm-controller does not deprecateupgrade.force: the CRD vendored atinternal/fluxinstall/manifests/fluxcd.yaml, which is what the pinned Flux 2.8.x ships, describes it as a replacement strategy with no deprecation marker.recreateis the neighbouring field that carries one.harbor, both suites. Both addressed their subject by document index. An index is a position, so a template that gains a document ahead of the one under test moves every later assertion onto a different object. Measured by inserting one document at the head of each template: the cleanup suite reddens outright, while two of the four jobservice tests keep passing: the two
notExistsones, satisfied against an object that never had the path. Every document these suites assert on is unique by kind within its template, so each test now names its subject. A selector that matches nothing fails loudly, including on thenotExiststests.Indices are also easy to get wrong by hand: helm-unittest numbers documents in template source order while
helm templateprints them sorted by kind, so the rendered output is not the order being counted.seaweedfs and cozystack-api. Both forbade a rule by naming it in full, and helm-unittest compares every field it is handed by value equality, so one respelling walks past. For seaweedfs that is not theoretical: ungating the cnpg block and adding a verb to the rule is the same grant and more, and the guard stayed green while a Client instance held delete on another app's Postgres cluster. The Role is now bounded by shape. A Client renders exactly two rules, both in the core group, and the cnpg grant is gated between them, so ungating it adds a third whatever its spelling.
For cozystack-api the count bound sits beside the existing named-rule assert rather than replacing it. Neither covers the other:
templates/rbac.yamlcarries no template directives, so the list is pinned at exactly sixteen rules from both sides, while a grant swapped in for an existing one leaves the count where it was and only the named-rule assert sees it. Measured on all three: append, delete, and swap.Screenshots
Not a UI change.
Downstream repositories
Walked the trigger map against the diff. It touches five files, all under
packages/*/*/tests/, and no chart, template, values file or tooling. Nothing in the map keys on test files.Release note
Summary by CodeRabbit