fix: reduce false positives for ServiceMesh capability conditions in DSCI check#55
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:
📝 WalkthroughWalkthroughAdds a table-driven test Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Code Quality & Security Findings
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/lint/checks/platform/dscinitialization/support.go (1)
75-83: Denylist approach may silently suppress future failure reasons.
isExpectedCapabilityServiceMeshAuthFalsetreats any reason exceptNotReadyas non-blocking. If a new failure reason is introduced (e.g.,Error,ConfigurationError,Timeout), it will silently be treated as expected/healthy.Consider using an explicit allowlist like
isExpectedCapabilityServiceMeshFalsedoes:Proposed allowlist approach
func isExpectedCapabilityServiceMeshAuthFalse(cond dsciStatusCondition) bool { return cond.Type == "CapabilityServiceMeshAuthorization" && cond.Status == string(metav1.ConditionFalse) && - cond.Reason != "NotReady" + (cond.Reason == "Removed" || cond.Reason == "Unmanaged" || cond.Reason == "MissingOperator") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/lint/checks/platform/dscinitialization/support.go` around lines 75 - 83, isExpectedCapabilityServiceMeshAuthFalse currently denies only "NotReady" as blocking and treats all other reasons as non-blocking, which can hide future real errors; update the function isExpectedCapabilityServiceMeshAuthFalse to use an explicit allowlist of safe non-blocking reasons (match the style of isExpectedCapabilityServiceMeshFalse), e.g., only return true when cond.Type == "CapabilityServiceMeshAuthorization", cond.Status == string(metav1.ConditionFalse) and cond.Reason is one of the approved reasons (Removed, Unmanaged, MissingOperator, etc.); ensure you reference the dsciStatusCondition parameter and mirror the allowlist pattern used by isExpectedCapabilityServiceMeshFalse so new unexpected reasons are treated as blocking.pkg/lint/checks/platform/dscinitialization/dscinitialization_readiness_test.go (1)
281-318: Missing test forCapabilityServiceMeshwithNotReadyreason.Tests cover
UnmanagedforCapabilityServiceMesh, but there's no test verifying thatCapabilityServiceMeshwithstatus: Falseandreason: NotReadyis correctly flagged as unhappy/blocking.This negative test would confirm the allowlist behavior (only
RemovedandUnmanagedare expected;NotReadyshould block).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/lint/checks/platform/dscinitialization/dscinitialization_readiness_test.go` around lines 281 - 318, Add a negative test mirroring TestDSCInitializationReadinessCheck_ReadyWithServiceMeshUnmanaged that constructs a DSCInitialization unstructured object but sets the CapabilityServiceMesh condition to {"type":"CapabilityServiceMesh","status":"False","reason":"NotReady",...}, create the target via testutil.NewTarget using dsciReadinessListKinds and that object, call dscinitialization.NewDSCInitializationReadinessCheck().Validate(...) and assert the result marks not-ready: expect a blocking/unhappy condition (ConditionTypeReady false, appropriate blocking Reason/message and Impact not ImpactNone). Use the same helper names (dsci, target, chk) so the test parallels the existing Unmanaged case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@pkg/lint/checks/platform/dscinitialization/dscinitialization_readiness_test.go`:
- Around line 281-318: Add a negative test mirroring
TestDSCInitializationReadinessCheck_ReadyWithServiceMeshUnmanaged that
constructs a DSCInitialization unstructured object but sets the
CapabilityServiceMesh condition to
{"type":"CapabilityServiceMesh","status":"False","reason":"NotReady",...},
create the target via testutil.NewTarget using dsciReadinessListKinds and that
object, call
dscinitialization.NewDSCInitializationReadinessCheck().Validate(...) and assert
the result marks not-ready: expect a blocking/unhappy condition
(ConditionTypeReady false, appropriate blocking Reason/message and Impact not
ImpactNone). Use the same helper names (dsci, target, chk) so the test parallels
the existing Unmanaged case.
In `@pkg/lint/checks/platform/dscinitialization/support.go`:
- Around line 75-83: isExpectedCapabilityServiceMeshAuthFalse currently denies
only "NotReady" as blocking and treats all other reasons as non-blocking, which
can hide future real errors; update the function
isExpectedCapabilityServiceMeshAuthFalse to use an explicit allowlist of safe
non-blocking reasons (match the style of isExpectedCapabilityServiceMeshFalse),
e.g., only return true when cond.Type == "CapabilityServiceMeshAuthorization",
cond.Status == string(metav1.ConditionFalse) and cond.Reason is one of the
approved reasons (Removed, Unmanaged, MissingOperator, etc.); ensure you
reference the dsciStatusCondition parameter and mirror the allowlist pattern
used by isExpectedCapabilityServiceMeshFalse so new unexpected reasons are
treated as blocking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: fbecc3a2-034c-4206-86e4-61ac39281397
📒 Files selected for processing (2)
pkg/lint/checks/platform/dscinitialization/dscinitialization_readiness_test.gopkg/lint/checks/platform/dscinitialization/support.go
lburgazzoli
force-pushed
the
dsci-authorino
branch
from
be5c661 to
a6530e8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/lint/checks/platform/dscinitialization/dscinitialization_readiness_test.go (2)
320-357: Missing test case:CapabilityServiceMeshwithMissingOperatorreason.Per PR objectives,
CapabilityServiceMeshonly exemptsRemovedandUnmanaged, whileCapabilityServiceMeshAuthorizationadditionally exemptsMissingOperator. Add a test verifying thatCapabilityServiceMeshwithreason: "MissingOperator"results inReady=False(blocking) to validate this asymmetry.Suggested test case
func TestDSCInitializationReadinessCheck_ReadyWithServiceMeshMissingOperator(t *testing.T) { g := NewWithT(t) dsci := &unstructured.Unstructured{ Object: map[string]any{ "apiVersion": resources.DSCInitialization.APIVersion(), "kind": resources.DSCInitialization.Kind, "metadata": map[string]any{ "name": "default-dsci", }, "status": map[string]any{ "phase": "Ready", "conditions": []any{ map[string]any{"type": "ReconcileComplete", "status": "True", "reason": "ReconcileCompleted", "message": "reconcile completed"}, map[string]any{"type": "CapabilityServiceMesh", "status": "False", "reason": "MissingOperator", "message": "ServiceMesh operator not found"}, }, }, }, } target := testutil.NewTarget(t, testutil.TargetConfig{ ListKinds: dsciReadinessListKinds, Objects: []*unstructured.Unstructured{dsci}, }) chk := dscinitialization.NewDSCInitializationReadinessCheck() result, err := chk.Validate(t.Context(), target) g.Expect(err).ToNot(HaveOccurred()) g.Expect(result.Status.Conditions).To(HaveLen(1)) g.Expect(result.Status.Conditions[0].Condition).To(MatchFields(IgnoreExtras, Fields{ "Type": Equal(check.ConditionTypeReady), "Status": Equal(metav1.ConditionFalse), "Reason": Equal(check.ReasonResourceUnavailable), "Message": ContainSubstring("CapabilityServiceMesh: ServiceMesh operator not found"), })) g.Expect(result.Status.Conditions[0].Impact).To(Equal(resultpkg.ImpactBlocking)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/lint/checks/platform/dscinitialization/dscinitialization_readiness_test.go` around lines 320 - 357, Add a new unit test mirroring TestDSCInitializationReadinessCheck_ReadyWithServiceMeshNotReady but using condition type "CapabilityServiceMesh" with reason "MissingOperator" to assert that NewDSCInitializationReadinessCheck().Validate(...) returns a single Condition where Condition.Type == check.ConditionTypeReady, Status == metav1.ConditionFalse, Reason == check.ReasonResourceUnavailable, the Message contains the provided "ServiceMesh operator not found" text, and Impact == resultpkg.ImpactBlocking; create the unstructured dsci object (same structure as dsci in the existing test), include it in the target via testutil.NewTarget with dsciReadinessListKinds, call chk := dscinitialization.NewDSCInitializationReadinessCheck() and validate the expectations exactly as in the suggested test.
281-474: Consider table-driven tests to reduce duplication.All five new test functions share identical structure with only varying input conditions and expected outcomes. Idiomatic Go favors table-driven tests for this pattern, improving maintainability when adding new reason/type combinations.
Table-driven test structure
func TestDSCInitializationReadinessCheck_CapabilityConditions(t *testing.T) { tests := []struct { name string conditionType string reason string message string expectReady metav1.ConditionStatus expectReason string expectImpact resultpkg.Impact expectContains string }{ { name: "ServiceMesh Unmanaged", conditionType: "CapabilityServiceMesh", reason: "Unmanaged", message: "ServiceMesh is set to Unmanaged", expectReady: metav1.ConditionTrue, expectReason: check.ReasonResourceAvailable, expectImpact: resultpkg.ImpactNone, expectContains: "DSCInitialization is ready", }, // ... additional cases } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { g := NewWithT(t) // ... common test logic using tc fields }) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/lint/checks/platform/dscinitialization/dscinitialization_readiness_test.go` around lines 281 - 474, Multiple near-duplicate tests (TestDSCInitializationReadinessCheck_ReadyWithServiceMeshUnmanaged, _NotReady, _AuthUnmanaged, _AuthMissingOperator, _AuthNotReady) should be consolidated into a single table-driven test (e.g. TestDSCInitializationReadinessCheck_CapabilityConditions) that iterates test cases with fields: name, conditionType, reason, message, expectReady (metav1.ConditionStatus), expectReason (check.Reason...), expectImpact (resultpkg.Impact), and expectContains (string). For each case create the same dsci unstructured object but inject tc.conditionType/tc.reason/tc.message into the status.conditions list, construct the target with dsci using dsciReadinessListKinds, call dscinitialization.NewDSCInitializationReadinessCheck() and chk.Validate(...), then perform the identical assertions against result.Status.Conditions[0] using the tc expected fields; keep reuse of NewWithT, testutil.NewTarget, and the existing assertion structure to preserve semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@pkg/lint/checks/platform/dscinitialization/dscinitialization_readiness_test.go`:
- Around line 320-357: Add a new unit test mirroring
TestDSCInitializationReadinessCheck_ReadyWithServiceMeshNotReady but using
condition type "CapabilityServiceMesh" with reason "MissingOperator" to assert
that NewDSCInitializationReadinessCheck().Validate(...) returns a single
Condition where Condition.Type == check.ConditionTypeReady, Status ==
metav1.ConditionFalse, Reason == check.ReasonResourceUnavailable, the Message
contains the provided "ServiceMesh operator not found" text, and Impact ==
resultpkg.ImpactBlocking; create the unstructured dsci object (same structure as
dsci in the existing test), include it in the target via testutil.NewTarget with
dsciReadinessListKinds, call chk :=
dscinitialization.NewDSCInitializationReadinessCheck() and validate the
expectations exactly as in the suggested test.
- Around line 281-474: Multiple near-duplicate tests
(TestDSCInitializationReadinessCheck_ReadyWithServiceMeshUnmanaged, _NotReady,
_AuthUnmanaged, _AuthMissingOperator, _AuthNotReady) should be consolidated into
a single table-driven test (e.g.
TestDSCInitializationReadinessCheck_CapabilityConditions) that iterates test
cases with fields: name, conditionType, reason, message, expectReady
(metav1.ConditionStatus), expectReason (check.Reason...), expectImpact
(resultpkg.Impact), and expectContains (string). For each case create the same
dsci unstructured object but inject tc.conditionType/tc.reason/tc.message into
the status.conditions list, construct the target with dsci using
dsciReadinessListKinds, call
dscinitialization.NewDSCInitializationReadinessCheck() and chk.Validate(...),
then perform the identical assertions against result.Status.Conditions[0] using
the tc expected fields; keep reuse of NewWithT, testutil.NewTarget, and the
existing assertion structure to preserve semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: ae066ee5-b105-4ced-9e5b-cf8841031b1a
📒 Files selected for processing (2)
pkg/lint/checks/platform/dscinitialization/dscinitialization_readiness_test.gopkg/lint/checks/platform/dscinitialization/support.go
✅ Files skipped from review due to trivial changes (1)
- pkg/lint/checks/platform/dscinitialization/support.go
lburgazzoli
force-pushed
the
dsci-authorino
branch
from
a6530e8 to
c0ba1d0
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/lint/checks/platform/dscinitialization/dscinitialization_readiness_test.go (1)
281-352: Test matrix aligns with implementation logic.The test cases correctly mirror the exemption rules:
CapabilityServiceMesh:MissingOperatoris blocking (not in the allowlist),Unmanagedis non-blocking.CapabilityServiceMeshAuthorization:MissingOperatoris non-blocking (onlyNotReadyis blocking).Consider adding a test case for
CapabilityServiceMeshwith reason"Removed"in this parameterized test for completeness, even thoughTestDSCInitializationReadinessCheck_ReadyWithRemovedCapabilitiescovers a similar scenario with both conditions present simultaneously.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/lint/checks/platform/dscinitialization/dscinitialization_readiness_test.go` around lines 281 - 352, Add a missing table-driven test case to TestDSCInitializationReadinessCheck_CapabilityConditions: include a case for conditionType "CapabilityServiceMesh" with reason "Removed" (message e.g. "ServiceMesh removed") and set expectedStatus/metav1.ConditionTrue, expectedReason check.ReasonResourceAvailable, expectedImpact resultpkg.ImpactNone and expectedMessage "DSCInitialization is ready"; update the tests slice in that function so the parameterized loop verifies the Removed exemption for CapabilityServiceMesh alongside the other cases.pkg/lint/checks/platform/dscinitialization/support.go (1)
75-86: Denylist approach warrants awareness of upstream changes.The
reason != "NotReady"denylist auto-accepts unknown reasons, which the comment justifies as reducing maintenance. However, if upstream introduces a new genuinely problematic reason (e.g.,"FailedConfiguration"), it will silently be treated as non-blocking until code is updated.Consider adding a log statement at debug level when an unrecognized reason is encountered so operators have visibility:
// Example: log.V(1).Info("CapabilityServiceMeshAuthorization condition has unrecognized reason", "reason", cond.Reason)This is a trade-off the team may accept; flagging for awareness only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/lint/checks/platform/dscinitialization/support.go` around lines 75 - 86, The function isExpectedCapabilityServiceMeshAuthFalse silently treats any False reason other than "NotReady" as non-blocking; add a debug-level log in isExpectedCapabilityServiceMeshAuthFalse to emit the encountered cond.Reason for visibility (use the repository's logger, e.g., log.V(1).Info or klog/V or logr as used elsewhere) when cond.Type == "CapabilityServiceMeshAuthorization" && cond.Status == string(metav1.ConditionFalse) && cond.Reason != "NotReady"; do not change the boolean return behavior, only add the debug log statement referencing cond.Reason and dsciStatusCondition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@pkg/lint/checks/platform/dscinitialization/dscinitialization_readiness_test.go`:
- Around line 281-352: Add a missing table-driven test case to
TestDSCInitializationReadinessCheck_CapabilityConditions: include a case for
conditionType "CapabilityServiceMesh" with reason "Removed" (message e.g.
"ServiceMesh removed") and set expectedStatus/metav1.ConditionTrue,
expectedReason check.ReasonResourceAvailable, expectedImpact
resultpkg.ImpactNone and expectedMessage "DSCInitialization is ready"; update
the tests slice in that function so the parameterized loop verifies the Removed
exemption for CapabilityServiceMesh alongside the other cases.
In `@pkg/lint/checks/platform/dscinitialization/support.go`:
- Around line 75-86: The function isExpectedCapabilityServiceMeshAuthFalse
silently treats any False reason other than "NotReady" as non-blocking; add a
debug-level log in isExpectedCapabilityServiceMeshAuthFalse to emit the
encountered cond.Reason for visibility (use the repository's logger, e.g.,
log.V(1).Info or klog/V or logr as used elsewhere) when cond.Type ==
"CapabilityServiceMeshAuthorization" && cond.Status ==
string(metav1.ConditionFalse) && cond.Reason != "NotReady"; do not change the
boolean return behavior, only add the debug log statement referencing
cond.Reason and dsciStatusCondition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3c0696ac-ddab-492c-bc8c-8cc77eff3f88
📒 Files selected for processing (2)
pkg/lint/checks/platform/dscinitialization/dscinitialization_readiness_test.gopkg/lint/checks/platform/dscinitialization/support.go
lburgazzoli
force-pushed
the
dsci-authorino
branch
from
c0ba1d0 to
b67505d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@pkg/lint/checks/platform/dscinitialization/dscinitialization_readiness_test.go`:
- Around line 281-362: Add a missing test row in
TestDSCInitializationReadinessCheck_CapabilityConditions for the
CapabilityServiceMeshAuthorization "Removed" case: create a test entry with
conditionType "CapabilityServiceMeshAuthorization", reason "Removed", message
"ServiceMesh removed", expectedStatus metav1.ConditionTrue, expectedReason
check.ReasonResourceAvailable, expectedImpact resultpkg.ImpactNone and
expectedMessage "DSCInitialization is ready" so the suite covers the
non-blocking removed scenario for ServiceMesh auth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e92ebf49-a1d3-4aa3-a1a6-27615aa37215
📒 Files selected for processing (2)
pkg/lint/checks/platform/dscinitialization/dscinitialization_readiness_test.gopkg/lint/checks/platform/dscinitialization/support.go
✅ Files skipped from review due to trivial changes (1)
- pkg/lint/checks/platform/dscinitialization/support.go
lburgazzoli
force-pushed
the
dsci-authorino
branch
from
b67505d to
dbd6a3a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/lint/checks/platform/dscinitialization/dscinitialization_readiness_test.go (1)
332-372: Add a fallback-reason auth case to lock the intended contract.The matrix skips an unknown
CapabilityServiceMeshAuthorizationfalse-reason case. Add one explicit row (e.g.,reason: "UnexpectedReason") with the expected readiness outcome so future logic changes don’t silently reintroduce false positives/negatives.Proposed test row
{ name: "ServiceMeshAuth/NotReady", conditionType: "CapabilityServiceMeshAuthorization", reason: "NotReady", message: "Authorino resource not ready", expectedStatus: metav1.ConditionFalse, expectedReason: check.ReasonResourceUnavailable, expectedImpact: resultpkg.ImpactBlocking, expectedMessage: "CapabilityServiceMeshAuthorization: Authorino resource not ready", }, + { + name: "ServiceMeshAuth/UnexpectedReason", + conditionType: "CapabilityServiceMeshAuthorization", + reason: "UnexpectedReason", + message: "unexpected auth condition reason", + expectedStatus: metav1.ConditionTrue, + expectedReason: check.ReasonResourceAvailable, + expectedImpact: resultpkg.ImpactNone, + expectedMessage: "DSCInitialization is ready", + }, }As per coding guidelines
**: REVIEW PRIORITIES: 3. Bug-prone patterns and error handling gaps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/lint/checks/platform/dscinitialization/dscinitialization_readiness_test.go` around lines 332 - 372, Add a new test row for an unknown false-reason to the existing cases: create an entry named "ServiceMeshAuth/UnexpectedReason" with conditionType "CapabilityServiceMeshAuthorization", reason "UnexpectedReason", message "Authorino resource not ready", and set expectedStatus to metav1.ConditionFalse, expectedReason to check.ReasonResourceUnavailable, expectedImpact to resultpkg.ImpactBlocking and expectedMessage to "CapabilityServiceMeshAuthorization: Authorino resource not ready" so the test matrix explicitly locks the fallback contract for unknown reasons.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@pkg/lint/checks/platform/dscinitialization/dscinitialization_readiness_test.go`:
- Around line 332-372: Add a new test row for an unknown false-reason to the
existing cases: create an entry named "ServiceMeshAuth/UnexpectedReason" with
conditionType "CapabilityServiceMeshAuthorization", reason "UnexpectedReason",
message "Authorino resource not ready", and set expectedStatus to
metav1.ConditionFalse, expectedReason to check.ReasonResourceUnavailable,
expectedImpact to resultpkg.ImpactBlocking and expectedMessage to
"CapabilityServiceMeshAuthorization: Authorino resource not ready" so the test
matrix explicitly locks the fallback contract for unknown reasons.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: feab7ad0-8275-4790-b268-737cf83a0660
📒 Files selected for processing (2)
pkg/lint/checks/platform/dscinitialization/dscinitialization_readiness_test.gopkg/lint/checks/platform/dscinitialization/support.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/lint/checks/platform/dscinitialization/support.go
There was a problem hiding this comment.
This test can be removed now so it is not duplicated.
Refactor the individual ServiceMesh capability condition tests into a single table-driven test, reducing duplication and making it easier to add new cases. Add missing negative test for CapabilityServiceMesh with MissingOperator reason to validate the asymmetry between the two condition types. The denylist approach (reason != "NotReady") for CapabilityServiceMeshAuthorization is intentionally kept over an explicit allowlist so that new benign reasons added upstream are automatically treated as non-blocking without a code change. Made-with: Cursor
lburgazzoli
force-pushed
the
dsci-authorino
branch
from
dbd6a3a to
ec5867c
Compare
2 participants
Split the single isRemovedCapabilityServiceMesh helper into two separate
functions with different exemption logic per condition type:
as a healthy intentional state (not flagged).
genuine problem; all other False reasons (Removed, Unmanaged,
MissingOperator) are non-blocking and no longer flagged.
Made-with: Cursor
Description
Testing
make test)make lint)Review checklist
Summary by CodeRabbit