HYPERFLEET-372 | fix: Adapter logging is reporting wrong error phase

[xueli181114]

• Moved Phase to CurrentPhase to make the variable clear usage
• Removed ErrorReason as no necessary to wrap the Error
• Changed Error to Errors as a map, each condition can have error happen. A map meet the usage better.

Summary by CodeRabbit

• Refactor
  • Restructured error reporting to track and report errors per execution phase for improved diagnostics.
  • Enhanced execution phase tracking with clearer visibility into the current execution state and failure points.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The pull request refactors error handling in the ExecutionResult structure, replacing separate Error and ErrorReason fields with a single Errors map keyed by ExecutionPhase to track phase-specific errors. The Phase field is renamed to CurrentPhase. The executor logic is updated to collect and store errors in the map during each phase execution, and all references in tests and documentation are updated to use the new field names and error-tracking structure.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

The changes are primarily structural refactoring with consistent patterns repeated across multiple files. However, the executor logic updates in executor.go require verification of error handling at each phase, and the distributed nature of field renames across five files (types.go, executor.go, tests, and documentation) necessitates checking for consistency and completeness.

Possibly related PRs

• HYPERFLEET-52 | feat: Implement executor for adapter #13: Directly modifies the same ExecutionResult structure with identical Phase→CurrentPhase and Error/ErrorReason→Errors map changes in the executor package.

Suggested labels

lgtm, approved

Suggested reviewers

• tirthct
• ciaranRoche

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 68.42% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title mentions fixing adapter logging and error phase reporting, which aligns with the core change of restructuring error tracking from a single Error field to a phase-specific Errors map and renaming Phase to CurrentPhase.

✨ Finishing touches

• 📝 Generate docstrings

---

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2acf251 and 1cfde58.

📒 Files selected for processing (6)

• internal/executor/README.md
• internal/executor/executor.go
• internal/executor/executor_test.go
• internal/executor/types.go
• test/integration/executor/executor_integration_test.go
• test/integration/executor/executor_k8s_integration_test.go

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: xueli181114
Repo: openshift-hyperfleet/hyperfleet-adapter PR: 13
File: internal/executor/post_action_executor.go:198-205
Timestamp: 2025-12-04T14:06:51.656Z
Learning: Logger refactor is tracked in HYPERFLEET-304 for the hyperfleet-adapter repository, which will address how CEL evaluation failures and similar errors are handled and logged.

🧬 Code graph analysis (4)

internal/executor/executor_test.go (2)

pkg/errors/error.go (1)

• Errors (77-95)

internal/executor/types.go (1)

• PhasePostActions (27-27)

test/integration/executor/executor_k8s_integration_test.go (2)

pkg/errors/error.go (1)

• Errors (77-95)

internal/executor/types.go (2)

• StatusSuccess (35-35)
• PhasePostActions (27-27)

test/integration/executor/executor_integration_test.go (2)

internal/executor/types.go (3)

• StatusSuccess (35-35)
• PhaseParamExtraction (21-21)
• PhasePostActions (27-27)

pkg/errors/error.go (1)

• Errors (77-95)

internal/executor/executor.go (1)

internal/executor/types.go (8)

• ExecutionResult (78-99)
• StatusFailed (37-37)
• PhaseParamExtraction (21-21)
• ExecutionPhase (17-17)
• StatusSuccess (35-35)
• PhasePreconditions (23-23)
• PhaseResources (25-25)
• PhasePostActions (27-27)

🔇 Additional comments (11)

internal/executor/README.md (1)

88-88: LGTM! Documentation accurately reflects the refactored error handling.

The documentation updates clearly describe the new CurrentPhase field and the Errors map structure. The changes align well with the code refactoring and improve clarity about per-phase error tracking.

Also applies to: 351-359, 400-401

internal/executor/types.go (1)

81-92: LGTM! Well-structured refactoring improves error tracking clarity.

The changes make the ExecutionResult structure more intuitive:

• CurrentPhase is clearer than Phase for indicating where execution ended
• Errors map[ExecutionPhase]error allows tracking multiple phase-specific errors, which is more flexible than a single Error field
• Removing ErrorReason reduces redundancy since error messages can contain this context

This is a breaking API change, but the improved structure justifies it.

test/integration/executor/executor_k8s_integration_test.go (1)

345-345: LGTM! Test assertions properly updated for new error structure.

All test assertions correctly reference:

• result.CurrentPhase instead of result.Phase
• result.Errors (map) instead of result.Error
• Proper map checks using NotEmpty assertions

The error logging format errors=%v appropriately displays the map structure.

Also applies to: 485-485, 772-774, 923-923

internal/executor/executor_test.go (2)

527-528: LGTM! Improved test assertions with better error messages.

The switch to assert.Equal for length checks provides clearer failure messages. The error map assertions properly use NotEmpty and phase-specific checks like NotNil(result.Errors[PhasePostActions]) validate the per-phase error structure.

Also applies to: 630-631, 702-711

---

811-827: Excellent validation of phase-specific errors.

This test properly validates the new error map structure by:

1. Checking result.Errors[executor.PhaseParamExtraction] exists
2. Verifying the error message contains expected content ("clusterId")

This ensures the map correctly tracks which phase failed.

test/integration/executor/executor_integration_test.go (2)

117-117: LGTM! Integration tests properly updated for new error tracking.

All integration test assertions correctly:

• Use result.Errors for error checking
• Include helpful context in assertion messages (errors=%v)
• Validate both success and failure scenarios

The consistent pattern across all tests ensures reliable validation of the refactored error handling.

Also applies to: 356-356, 463-463, 649-650

---

689-691: Well-structured phase-specific error validation.

These tests properly verify:

• result.CurrentPhase indicates where execution ended
• Phase-specific errors are accessible via result.Errors[PhaseParamExtraction]
• Error messages contain relevant details (like missing field names)

This validates that the error map correctly tracks which phase encountered issues.

Also applies to: 756-758, 811-827

internal/executor/executor.go (4)

57-63: LGTM! Proper initialization and error handling.

The implementation correctly:

• Initializes Errors as make(map[ExecutionPhase]error) (line 84)
• Sets CurrentPhase to PhaseParamExtraction early (line 85)
• Records parse errors in the appropriate phase: Errors[PhaseParamExtraction] (line 62)

The consistent initialization pattern ensures no nil map panics.

Also applies to: 81-86

---

93-99: LGTM! Phase-based error handling flow is well-structured.

The execution flow properly:

1. Updates CurrentPhase at each phase transition (lines 103, 131)
2. Records errors in the phase-specific map entry (lines 94, 111, 139)
3. Maintains both Errors map and ExecutionContext.Adapter metadata (for CEL/templates)
4. Continues to post-actions for error reporting even when earlier phases fail

The logic at lines 110-117 correctly handles precondition failures by setting both error tracking (Errors[PhasePreconditions]) and business outcome flags (ResourcesSkipped), then continues to post-actions for status reporting.

Also applies to: 103-127, 131-151

---

159-177: LGTM! Post-action phase and final logging properly updated.

Post-action handling correctly:

• Sets CurrentPhase to PhasePostActions (line 159)
• Records post-action errors in the map (line 166)
• Includes comprehensive final logging showing either resources skipped info or aggregated errors (lines 183-185)

The final log messages provide clear visibility: resources_skipped=%t reason=%s for success or event_execution_errors=%v for failures, showing all phase-specific errors.

Also applies to: 183-186

---

218-224: LGTM! Simplified handler logging.

The handler now logs a consolidated "Event processed" message instead of phase-specific details. This is cleaner since detailed phase information is already logged within Execute().

The ACK-everything strategy (return nil) remains appropriate for preventing infinite retry loops on non-recoverable errors.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[86254860]

Errors here is a bit confusing. Would PhaseErrors be clearer?

[xueli181114]

Good suggestion. Errors means the execution error. There is no other errors defined in this struct to cause confusion. Let's keep it until we got further errors introduced.

[86254860]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 86254860

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [86254860]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment