test(backupstrategy-controller): anchor the metrics-absence guard to its container - #3605
Conversation
…its container The two notContains assertions in the metrics.enabled=false case address the controller by index. notContains passes whenever its path resolves to something that lacks the forbidden content, and a different container at that index qualifies, so a sidecar inserted ahead of the controller leaves both assertions green while the flag they forbid comes back on the real container. Assert the container's name at the same index first. That fails when the index stops pointing at the controller, which is the case the negatives structurally cannot see. Verified by rendering a sidecar ahead of the controller: before this change the case stayed green and only unrelated tests in the same file went red, by accident rather than by design; after it, the anchor fails. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
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)
📝 WalkthroughWalkthroughThe metrics-disabled deployment test now verifies the ChangesBackupstrategy controller deployment tests
Estimated code review effort: 1 (Trivial) | ~5 minutes 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 |
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM
Summary
The metrics.enabled=false suite asserted metrics absence with two notContains on spec.template.spec.containers[0]. notContains is satisfied by any path that resolves to something without the forbidden content — including the wrong container. If a sidecar were ever inserted ahead of the controller, it would take index 0, the controller would shift to index 1, and both negatives would stay green even though --metrics-bind-address=:8443 had come back on the real controller. That is a guard that passes exactly when it should fail.
This PR adds an equal anchor pinning containers[0].name to backupstrategy-controller ahead of the two negatives, converting the positional assumption into an explicit, fail-loud precondition.
Findings
None blocking.
Verification
packages/system/backupstrategy-controller/templates/deployment.yaml:25-26— the controller is the sole container and sits at index 0 today; there are noinitContainersand no conditional container blocks ahead of it, so the newequalpasses on the current template (no false failure).- The anchor is placed only in the negative-assert test (
tests/deployment_test.yaml:37-39). The two sibling tests use positivecontainsoncontainers[0], which already fail loud on the wrong container, so they correctly do not need the anchor — scoping is right, not an omission. helm unitteston the chart: 11/11 tests pass, all 3 suites green — the anchor does not false-fail the legitimatemetrics.enabled=falserender.
Caveats
- Phase 5b: N/A — test-only change, no chart-output or runtime surface reaches existing customers or fresh installs.
Anchoring index-addressed negative assertions to the identity of the element they name is the correct idiom in helm-unittest (which has no by-name container selector). The 6-line explanatory comment documents a genuinely non-obvious testing subtlety and stays under the template-comment-bloat threshold; it belongs here.
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM
The anchor assertion is correctly targeted, and its value was confirmed by executing the exact regression scenario in both directions rather than by reasoning about it.
Caveats
- Phase 5b (existing-customer upgrade / fresh install) and Phase 5c (config-toggle matrix) are not applicable: the diff touches only a
helm-unittestassertion file (packages/system/backupstrategy-controller/tests/deployment_test.yaml), no chart template, value, or schema changed, so there is no render/upgrade surface to exercise.
Verification performed
- Rendered the chart's baseline suite: 3 suites / 11 tests green before touching anything.
- Reproduced the exact failure mode the PR describes by mutating a scratch copy of
templates/deployment.yaml(added amutation-test-sidecarcontainer ahead ofbackupstrategy-controller, and made the metrics arg/port render unconditionally to simulate the typo regression returning):- With the PR's
equalanchor in place, themetrics.enabled=falsecase fails as expected (asserts[0] equal fail, expectedbackupstrategy-controller, actualmutation-test-sidecar). - With the anchor removed (pre-fix shape), the same mutated tree makes the
metrics.enabled=falsecase pass silently — the twonotContainsassertions read the sidecar's emptyargs/portsand see "absent," while the real controller container one index over still renders the forbidden metrics flag. This reproduces the vacuous-pass the PR is fixing, not merely restates the claim. - Both other cases in the file (
metrics.enabled=true, "projector reads its source Secret…") independently redden under the same mutation, confirming they already defend themselves via positivecontains, exactly as the PR body states.
- With the PR's
- Confirmed
templates/deployment.yaml:26names the single existing containerbackupstrategy-controller, matching the anchor's expected value; no drift between the assertion and the current render. - Independently verified the PR's rejected alternative ("switch to a name-filter instead of an index"): a
notContainsassertion againstcontainers[?(@.name=="<name-that-does-not-exist>")]passes silently in this repo'shelm-unittest(v1.0.3), confirming a JSONPath-filternotContainsis equally vacuous when the filter matches zero containers. The stated reason for not taking that route is accurate, not just asserted. - Confirmed the cited precedents (
packages/system/opensearch-operator/tests/leader_election_test.yaml,packages/system/dashboard/tests/gatekeeper_test.yaml) already anchor index-addressed assertions with a precedingequalon the container name at the same index; this PR brings the third package in line with an existing, established idiom rather than introducing a new one. - Restored both mutated files afterward; working tree is clean against
HEAD(8aadbd5).
No regression is possible on any supported path: the change only strengthens a test assertion and touches no rendered manifest, value, or schema.
What this PR does
The
metrics.enabled=falsecase indeployment_test.yamlguards against themetrics.enabletypo regression by asserting that the metrics arg and port are absent. Both assertions address the controller by index, andnotContainspasses whenever its path resolves to something that lacks the forbidden content. A different container at that index qualifies, so a sidecar placed ahead of the controller leaves the guard green while the flag it exists to forbid comes back on the real container.This adds an
equaloncontainers[0].nameahead of the two negatives. It fails when the index stops pointing at the controller, which is the case the negatives structurally cannot see.Verified rather than argued. Rendering a sidecar ahead of the controller against the current tests leaves the case green and reddens only the two neighbouring tests, which use positive
containsand defend themselves; the guard that exists for this scenario is not among them. With the anchor, the case fails on the anchor.This is not a new convention.
packages/system/opensearch-operator/tests/leader_election_test.yamlandpackages/system/dashboard/tests/gatekeeper_test.yamlalready anchor index-addressed negatives the same way, the second with the reasoning spelled out in a comment. This brings one more package in line with what the tree already does.What this PR does not do
It does not switch the assertions to a name filter. helm-unittest resolves
containers[?(@.name=="...")], butnotContainsagainst a filter that matches nothing passes just as quietly, so that trade swaps "wrong container" for "no container" inside the same family of silent green.It does not touch the other two cases in the file. Both are positive
contains, both went red under the same injection, so they anchor themselves.Tests
The change is itself a test fix. Package suite is green: 11 tests, 3 suites.
Screenshots
None. This changes a test file.
Downstream repositories
Walked the trigger map against the diff. One helm-unittest file, nine added lines, no chart output changes.
Release note
Summary by CodeRabbit