feat: add revision result summary utility#393
Conversation
Co-authored-by: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Co-authored-by: Per Goncalves da Silva <pegoncal@redhat.com> Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
WalkthroughIntroduces a new Go utility module for generating concise, human-readable summaries from boxcutter reconciliation and teardown results. Adds functions to summarize revision results, phases, objects, probe outcomes, and teardown operations, with comprehensive unit tests covering various scenarios including validation errors, object collisions, probe failures, and in-transition states. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #393 +/- ##
==========================================
+ Coverage 68.33% 68.82% +0.49%
==========================================
Files 34 35 +1
Lines 2691 2836 +145
==========================================
+ Hits 1839 1952 +113
- Misses 728 745 +17
- Partials 124 139 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
util/summary.go (3)
1-9: Package name and imports look appropriate for this utility module.The static analysis hint about "util" being a meaningless package name is valid from a purist standpoint, but this is a common and accepted pattern for general-purpose utility code. If the project prefers more descriptive names, consider renaming to something like
summaryorreport.
121-136: Redundant call toProbeResults()- store the result in a variable.
obj.ProbeResults()is called twice in this block (line 123 in the for-range and again at line 132). Store the result once and reuse it.default: + probes := obj.ProbeResults() // Check probe results - for probeType, probeResult := range obj.ProbeResults() { + for probeType, probeResult := range probes { switch probeResult.Status { case types.ProbeStatusFalse: probeFailures = append(probeFailures, fmt.Sprintf("%q probe for %s failed", probeType, objInfo)) case types.ProbeStatusUnknown: probeUnknowns = append(probeUnknowns, fmt.Sprintf("%q probe for %s unknown", probeType, objInfo)) } } - probes := obj.ProbeResults() if len(probes) == 0 || allProbesSuccessful(probes) { successCount++ }
141-172: Consider extracting the "limit to first 3" logic into a helper function.The pattern for limiting displayed items to first 3 is repeated three times. A helper function would reduce duplication:
func formatLimitedList(items []string, singular string, limit int) string { if len(items) > limit { return fmt.Sprintf("%d %s(s) [showing first %d: %s]", len(items), singular, limit, strings.Join(items[:limit], ", ")) } return fmt.Sprintf("%d %s(s): %s", len(items), singular, strings.Join(items, ", ")) }util/summary_test.go (1)
86-119: Good use of interface assertion.The compile-time interface assertion at line 86 is a Go best practice. Consider adding similar assertions for the other mock types for consistency.
+var _ machinery.RevisionResult = mockRevisionResult{} +var _ machinery.PhaseResult = mockPhaseResult{} var _ machinery.ObjectResult = &mockObjectResult{} +var _ machinery.RevisionTeardownResult = mockRevisionTeardownResult{} +var _ machinery.PhaseTeardownResult = mockPhaseTeardownResult{}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
util/summary.go(1 hunks)util/summary_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
util/summary.go (6)
machinery/revision.go (1)
RevisionResult(58-74)machinery/phases.go (2)
PhaseResult(213-231)PhaseTeardownResult(67-81)machinery/results.go (3)
ObjectResult(14-34)Action(278-278)ActionCollision(293-293)machinery/objects.go (1)
Object(60-63)probing/probe.go (1)
Status(17-17)machinery/types/probing.go (4)
ProbeStatusFalse(24-24)ProbeStatusUnknown(26-26)ProbeResultContainer(38-38)ProbeStatusTrue(22-22)
🪛 golangci-lint (2.5.0)
util/summary.go
[warning] 1-1: var-naming: avoid meaningless package names
(revive)
util/summary_test.go
[warning] 1-1: var-naming: avoid meaningless package names
(revive)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-unit-int
🔇 Additional comments (10)
util/summary.go (4)
11-49: LGTM!The
SummarizeRevisionResultfunction is well-structured with proper nil handling, clear logic flow, and appropriate default messaging.
51-97: LGTM!The phase summarization logic correctly categorizes phases and provides a clear summary. The fallback to "unnamed" for empty phase names is a good defensive measure.
187-218: LGTM!The
getObjectInfoandallProbesSuccessfulhelper functions are clean, handle nil inputs appropriately, and follow Go idioms.
220-283: LGTM!The teardown summarization functions are well-structured with proper nil handling and clear logic. The fallback message "teardown in progress" when no specific details are available is a good default.
util/summary_test.go (6)
16-85: LGTM!The mock implementations for
mockRevisionResultandmockPhaseResultare well-structured and implement the necessary interface methods. TheString()methods returning placeholder text is appropriate for mocks.
121-183: LGTM!The teardown mock implementations are adequate for the current test coverage. The
Gone()andWaiting()methods aren't used bysummarizeTeardownPhases, so their simplified logic doesn't affect test validity.
184-253: LGTM!The revision result tests are comprehensive, covering nil input, success, validation errors, collisions, and probe states. Good use of
t.Parallel()for concurrent test execution.
339-378: Good test for the "showing first 3" truncation behavior.This test correctly validates that when there are more than 3 collisions, the summary shows the count and indicates truncation.
390-430: LGTM!The teardown tests cover the key scenarios: complete, waiting phases, and incomplete phases. Assertions correctly validate the expected output strings.
432-474: LGTM!The
getObjectInfotests thoroughly cover namespace-scoped objects, cluster-scoped objects, and nil handling.
|
Could this one be accepted? c/c @perdasilva |
|
Looks good to me, thank you for the ping :) |
3 participants
Summary
This effort came from @camilamacedo86. I'm just shepherding it over into the Boxcutter library with some refactoring for the changes in the probe interface (I've added the probe unknown case as well).
This PR introduces utility functions to format boxcutter reconciliation and teardown reports into concise, human-readable summaries. This makes debugging easier without enabling verbose V(1) logging and solves the scenarios reported over huge logs
Example Scenarios
Scenario 1: Object Collision During Reconcile
What happens: A ServiceAccount already exists and is managed by a different controller.
Controller Logs:
{ "level": "error", "ts": "2025-11-20T14:32:18.445Z", "logger": "cluster-extension-revision", "msg": "revision reconcile failed", "error": "apply failed: server rejected request", "summary": "phases with issues: deploy: 2 collision(s): ServiceAccount test-ns/argocd-operator-controller-manager, ServiceAccount test-ns/argocd-operator-metrics-reader; status: incomplete", "controller": "clusterextensionrevision", "controllerGroup": "olm.operatorframework.io", "controllerKind": "ClusterExtensionRevision", "ClusterExtensionRevision": { "name": "argocd-operator-1" }, "namespace": "", "name": "argocd-operator-1", "reconcileID": "8f7e3d9a-b2c1-4e5f-a6d7-1c8e9f0a2b3d" }Key Information:
error: The original error from boxcuttersummary: Human-readable explanation showing 2 colliding ServiceAccountsScenario 2: Multiple Collisions (Shows First 3)
What happens: Many resources collide, but we limit output to first 3.
Controller Logs:
{ "level": "error", "ts": "2025-11-20T14:33:45.678Z", "logger": "cluster-extension-revision", "msg": "revision reconcile failed", "error": "apply failed: multiple conflicts detected", "summary": "phases with issues: deploy: 5 collision(s) [showing first 3: ConfigMap test-ns/operator-config, ServiceAccount test-ns/operator-sa, Secret test-ns/operator-token]; status: incomplete", "controller": "clusterextensionrevision", "ClusterExtensionRevision": { "name": "my-operator-1" } }Key Information:
Scenario 3: Probe Failure During Rollout
What happens: Deployment is created but not becoming ready.
Controller Logs:
{ "level": "error", "ts": "2025-11-20T14:35:22.891Z", "logger": "cluster-extension-revision", "msg": "revision reconcile failed", "error": "rollout incomplete: timeout waiting for resources", "summary": "phases with issues: deploy: 1 probe failure(s): 'progress' probe for Deployment test-ns/argocd-operator-controller-manager failed; status: in transition", "controller": "clusterextensionrevision", "controllerGroup": "olm.operatorframework.io", "controllerKind": "ClusterExtensionRevision", "ClusterExtensionRevision": { "name": "argocd-operator-1" } }Key Information:
Scenario 4: Probe Unknown During Rollout
What happens: Deployment is created but status could not yet be observed.
Controller Logs:
{ "level": "error", "ts": "2025-11-20T14:35:22.891Z", "logger": "cluster-extension-revision", "msg": "revision reconcile failed", "error": "rollout incomplete: timeout waiting for resources", "summary": "phases with issues: deploy: 1 probe unknowns(s): 'progress' probe unknown for Deployment test-ns/argocd-operator-controller-manager; status: in transition", "controller": "clusterextensionrevision", "controllerGroup": "olm.operatorframework.io", "controllerKind": "ClusterExtensionRevision", "ClusterExtensionRevision": { "name": "argocd-operator-1" } }Key Information:
Scenario 5: Validation Error
What happens: Preflight validation fails (e.g., CRD upgrade safety check).
Controller Logs:
{ "level": "error", "ts": "2025-11-20T14:37:10.234Z", "logger": "cluster-extension-revision", "msg": "revision reconcile failed", "error": "validation failed", "summary": "validation error: CRD validation failed for CustomResourceDefinition mycrds.example.com; phases with issues: preflight: validation error", "controller": "clusterextensionrevision", "ClusterExtensionRevision": { "name": "example-operator-2" } }Key Information:
Scenario 6: Teardown Blocked by Finalizer
What happens: Trying to teardown but resource has blocking finalizer.
Controller Logs:
{ "level": "error", "ts": "2025-11-20T14:38:45.123Z", "logger": "cluster-extension-revision", "msg": "revision teardown failed", "error": "teardown incomplete: resources still exist", "summary": "waiting on phases: deploy, cleanup; incomplete phases: deploy; 1 phase(s) completed", "controller": "clusterextensionrevision", "controllerGroup": "olm.operatorframework.io", "controllerKind": "ClusterExtensionRevision", "ClusterExtensionRevision": { "name": "argocd-operator-1" } }Key Information:
Scenario 7: Successful Teardown (Info Level)
What happens: Teardown completes successfully.
Controller Logs:
{ "level": "info", "ts": "2025-11-20T14:40:12.567Z", "logger": "cluster-extension-revision", "msg": "teardown report", "report": "teardown completed successfully", "controller": "clusterextensionrevision", "controllerGroup": "olm.operatorframework.io", "controllerKind": "ClusterExtensionRevision", "ClusterExtensionRevision": { "name": "argocd-operator-1" } }Key Information:
Scenario 8: Mixed Issues in Multiple Phases
What happens: Multiple phases have different types of issues.
Controller Logs:
{ "level": "error", "ts": "2025-11-20T14:42:30.890Z", "logger": "cluster-extension-revision", "msg": "revision reconcile failed", "error": "multiple failures during rollout", "summary": "phases with issues: preflight: validation error, deploy: 1 collision(s): ServiceAccount test-ns/operator-sa, configure: 1 failed object(s): ConfigMap test-ns/config (action: update); status: incomplete", "controller": "clusterextensionrevision", "ClusterExtensionRevision": { "name": "complex-operator-1" } }Motivation:
https://issues.redhat.com/browse/OCPBUGS-62964
Change Type
New Feature
Check List Before Merging
Additional Information