HYPERFLEET-755 - fix: correct Health CEL expression in adapter failure test

[tzhou5]

Summary by CodeRabbit

• New Features
  • Clearer adapter health/status reporting to distinguish execution failures, skipped-resource cases, and healthy runs.
  • Improved reason selection so execution failures return a specific "ExecutionFailed" reason, skipped resources yield an explicit skip reason, and successful runs show a default healthy reason.
  • More informative status messages: failure messages surface adapter errors, skipped-resource scenarios are explicit, and successes show a default success message.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c65e9e40-549f-4e27-9e3a-b1240ee778c5

📥 Commits

Reviewing files that changed from the base of the PR and between 9b437fe and 79847ae.

📒 Files selected for processing (1)

• testdata/adapter-configs/cl-precondition-error/adapter-task-config.yaml

🚧 Files skipped from review as they are similar to previous changes (1)

• testdata/adapter-configs/cl-precondition-error/adapter-task-config.yaml

---

Walkthrough

The change updates a YAML adapter task configuration for post.payloads[].build.conditions[] entries of type Health. status.expression now evaluates to "True" only when adapter.?executionStatus.orValue("") == "success" and not when adapter.?resourcesSkipped.orValue(false) == true with a non-empty adapter.?errorReason; otherwise it yields "False". reason.expression now returns "ExecutionFailed" if executionStatus != "success", else returns adapter.?errorReason when non-empty, otherwise "Healthy". message.expression was rewritten as a nested conditional that returns error-related messages when execution failed or an errorMessage is present, else a success message.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: fixing a Health CEL expression in an adapter failure test configuration, directly aligned with the changeset.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

📝 Coding Plan

• Generate coding plan for human review comments

---

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

[xueli181114]

The Health CEL fix looks correct — properly handles resourcesSkipped for status, reason, and message. The ternary chain is clear and readable.

One concern: 5 other adapter-task-config files still use the old Health CEL pattern (without resourcesSkipped). Should they be updated too for consistency?

• cl-deployment/adapter-task-config.yaml — old pattern
• cl-invalid-resource/adapter-task-config.yaml — old pattern
• cl-job/adapter-task-config.yaml — old pattern
• cl-namespace/adapter-task-config.yaml — old pattern
• np-configmap/adapter-task-config.yaml — old pattern
• cl-maestro/adapter-task-config.yaml — already has new pattern

If only cl-precondition-error needs this because it's the only test that exercises the resourcesSkipped path, then this is fine as-is. But if these configs represent the standard Health condition template, they should all be aligned.

[tzhou5]

The Health CEL fix looks correct — properly handles resourcesSkipped for status, reason, and message. The ternary chain is clear and readable.

One concern: 5 other adapter-task-config files still use the old Health CEL pattern (without resourcesSkipped). Should they be updated too for consistency?

• cl-deployment/adapter-task-config.yaml — old pattern
• cl-invalid-resource/adapter-task-config.yaml — old pattern
• cl-job/adapter-task-config.yaml — old pattern
• cl-namespace/adapter-task-config.yaml — old pattern
• np-configmap/adapter-task-config.yaml — old pattern
• cl-maestro/adapter-task-config.yaml — already has new pattern

If only cl-precondition-error needs this because it's the only test that exercises the resourcesSkipped path, then this is fine as-is. But if these configs represent the standard Health condition template, they should all be aligned.

These 5 adapters are "happy path" configs where preconditions always pass, so resourcesSkipped is always false and the old expression produces the same result. No functional impact on existing tests.

That said, I agree they should be aligned for consistency, while I am concerned that modifying other configurations within the HYPERFLEET-755 of my auto-case might lead to misunderstandings.

[xueli181114]

The Health CEL status expression has a logic issue with max-age reconciliation events.

Scenario: Sentinel publishes an event because max age exceeded (generation unchanged). The adapter receives it, correctly determines there's nothing to do, and sets executionStatus=success with resourcesSkipped=true.

The new status expression:

adapter.?executionStatus.orValue("") == "success" && !adapter.?resourcesSkipped.orValue(false) ? "True" : "False"

Evaluates: true && !true → false → Health = "False"

This incorrectly marks the resource as unhealthy when the adapter legitimately skipped work because nothing changed. The resourcesSkipped flag doesn't distinguish between:

• Legitimate skip — no work needed (max age reconciliation, everything in sync)
• Problem skip — precondition failure prevented processing

The old expression (executionStatus == "success" ? "True" : "False") didn't have this issue.

Suggestion: Either:

1. Only treat resourcesSkipped as unhealthy when there's also an errorReason or errorMessage
2. Use a separate flag like preconditionFailed to distinguish error skips from no-op skips
3. Or consider: executionStatus == "success" && !(resourcesSkipped && errorReason != "") — skip is only unhealthy when accompanied by an error

[rafabene]

Category: Bug

The message.expression doesn't account for the new ExecutionFailed reason path. When
executionStatus != "success" and errorMessage is empty, the message defaults to "All adapter operations completed successfully" — which contradicts the "False" status and
"ExecutionFailed" reason.

The cl-maestro adapter config already handles this with a 3-way ternary. Consider matching
that pattern:

message:
  expression: |
    adapter.?executionStatus.orValue("") != "success"
      ? adapter.?errorMessage.orValue("Adapter execution failed")
      : adapter.?errorMessage.orValue("") != ""
        ? adapter.?errorMessage.orValue("")
        : "All adapter operations completed successfully"

This way, execution failures always get a meaningful message even if errorMessage is empty.

[tzhou5]

Updated

[xueli181114]

The updated CEL expressions correctly handle all three scenarios:

1. Normal success (executionStatus=success, no skip) → Health True ✓
2. Legitimate skip (executionStatus=success, resourcesSkipped=true, no errorReason) → Health True ✓
3. Precondition error (executionStatus=success, resourcesSkipped=true, errorReason set) → Health False ✓
4. Execution failure (executionStatus != success) → Health False, reason ExecutionFailed, meaningful fallback message ✓

The logic is consistent across status, reason, and message expressions.

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xueli181114

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 [xueli181114]

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