fix(controller): set workflow failed condition on validation failure by kavix · Pull Request #3877 · openchoreo/openchoreo

kavix · 2026-06-16T20:33:36Z

When a workflow validation fails or workflow is not found, the controller previously only set the WorkflowCompleted condition to true. This caused the UI to continue showing the workflow in a pending state.

This PR explicitly sets WorkflowFailed to true and WorkflowRunning to false in these error cases to properly surface the failure.

Closes #2948

When a workflow validation fails or workflow is not found, the controller previously only set the WorkflowCompleted condition to true. This caused the UI to continue showing the workflow in a pending state. This commit explicitly sets WorkflowFailed to true and WorkflowRunning to false in these error cases to properly surface the failure. Closes openchoreo#2948 Signed-off-by: kavix <[email protected]>

coderabbitai · 2026-06-16T20:33:58Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 565919cf-b328-481b-ad4d-5c29c52251e8

📥 Commits

Reviewing files that changed from the base of the PR and between 195fccc and 44b0e4d.

📒 Files selected for processing (2)

internal/controller/workflowrun/controller_conditions.go
internal/controller/workflowrun/controller_unit_test.go

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

internal/controller/workflowrun/controller_conditions.go

📝 Walkthrough

Changed Files & Breakdown

3 files changed (all under internal/)
- internal/: 3
  - internal/controller/workflowrun/controller_conditions.go
  - internal/controller/workflowrun/controller_integration_test.go
  - internal/controller/workflowrun/controller_unit_test.go
- api/: 0
- config/: 0
- pkg/: 0
- install/: 0
- docs/: 0
- openapi/: 0
- agents/: 0
- make/: 0
- cmd/: 0
- samples/: 0

API/CRD Surface Changes

None (no schema/field changes to API/CRDs).
Change is limited to controller semantics for WorkflowRun.status.conditions (which conditions/reasons/messages are set).
Compatibility risk: Low (consumer/UI behavior change via status-condition interpretation; no API contract changes).

What Changed (controller condition semantics)

Workflow not found (cluster lookup failure)

In setWorkflowNotFoundCondition:

ConditionWorkflowRunning=False (reason ReasonWorkflowRunning, message “Workflow is not found in the cluster”)
ConditionWorkflowFailed=True (reason ReasonWorkflowFailed, message “Workflow is not found in the cluster”)
ConditionWorkflowCompleted=True (reason ReasonWorkflowFailed, message “Workflow is not found in the cluster”)

Component validation failure

In setComponentValidationFailedCondition(message):

ConditionWorkflowRunning=False (reason ReasonWorkflowRunning, message = provided message)
ConditionWorkflowFailed=True (reason ReasonComponentValidationFailed, message = provided message)
ConditionWorkflowCompleted=True (reason ReasonComponentValidationFailed, message = provided message)

Tests (updated)

Integration tests

internal/controller/workflowrun/controller_integration_test.go

Updated “Workflow not found after pending condition set” to assert:
- ConditionWorkflowCompleted=True + reason ReasonWorkflowFailed
- ConditionWorkflowFailed=True
- ConditionWorkflowRunning=False
Updated “WorkflowRun with only project label fails validation” to assert:
- ConditionWorkflowFailed=True
- ConditionWorkflowRunning=False
- (alongside the existing ConditionWorkflowCompleted/reason/message checks)

Unit tests

internal/controller/workflowrun/controller_unit_test.go

Updated TestConditionFunctions → setWorkflowNotFoundCondition to assert:
- ConditionWorkflowRunning=False
- ConditionWorkflowFailed=True (reason ReasonWorkflowFailed)
Updated TestSetComponentValidationFailedCondition to assert:
- ConditionWorkflowRunning=False (reason ReasonWorkflowRunning)
- ConditionWorkflowFailed=True (reason ReasonComponentValidationFailed)
- ConditionWorkflowCompleted=True (reason ReasonComponentValidationFailed)

Risk Hotspots

Reconciliation/terminal-condition correctness (Medium): controller now sets both WorkflowCompleted=True and WorkflowFailed=True (plus WorkflowRunning=False) for validation/missing-workflow errors; consumers that previously relied on WorkflowCompleted alone must interpret WorkflowFailed correctly.
UI/consumer interpretation of status conditions (Medium): fix depends on UI/clients rendering “failed” when ConditionWorkflowFailed=True and not showing indefinite “pending” when workflows are actually terminally failed.
No authn/authz/RBAC/secrets/installer hotspot: no changes to authentication/authorization, RBAC bindings, secret handling, or install/upgrade paths—only controller-managed status condition wiring and corresponding assertions.

Walkthrough

Two condition-setter functions in the WorkflowRun controller are updated to emit additional status conditions on failure. setWorkflowNotFoundCondition gains a ConditionWorkflowFailed=True condition, and setComponentValidationFailedCondition gains both ConditionWorkflowRunning=False and ConditionWorkflowFailed=True conditions alongside the existing ConditionWorkflowCompleted conditions. Tests are updated to verify these new conditions are set correctly.

Changes

WorkflowRun failure condition completeness

Layer / File(s)	Summary
Workflow-not-found and component-validation failure condition setters `internal/controller/workflowrun/controller_conditions.go`	`setWorkflowNotFoundCondition` now sets `ConditionWorkflowFailed=True` with `ReasonWorkflowFailed` for the deleted-workflow case. `setComponentValidationFailedCondition` now sets `ConditionWorkflowRunning=False` and `ConditionWorkflowFailed=True` with `ReasonComponentValidationFailed` in addition to the existing `ConditionWorkflowCompleted=True`.
Test assertions for new failure conditions `internal/controller/workflowrun/controller_unit_test.go`, `internal/controller/workflowrun/controller_integration_test.go`	Unit tests and integration tests for workflow-not-found and component-validation-failure scenarios now assert that `ConditionWorkflowFailed` is `True` and `ConditionWorkflowRunning` is `False` alongside existing condition checks.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description lacks the structured template format required by the repository, missing sections like Purpose, Approach, Checklist, and Remarks.	Update the description to follow the required template format including Purpose, Approach, Related Issues, Checklist, and Remarks sections.
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: setting the workflow failed condition when validation fails, which directly addresses the PR's core objective.
Linked Issues check	✅ Passed	The code changes successfully implement the fix for issue `#2948` by explicitly setting WorkflowFailed=true and WorkflowRunning=false when validation fails or workflow is not found.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the workflow validation failure condition behavior; no out-of-scope modifications were detected.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Stopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a @coderabbit review after the pipeline has finished.

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.

❤️ Share

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

coderabbitai

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/controller/workflowrun/controller_conditions.go`:
- Around line 173-186: The code in controller_conditions.go at lines 173-186
sets three conditions (ConditionWorkflowRunning=False,
ConditionWorkflowFailed=True, and ConditionWorkflowCompleted=True), but the
tests do not fully validate all three. Update
TestSetComponentValidationFailedCondition to expect and verify all three
conditions with their correct status values instead of just one condition.
Update the integration test at controller_integration_test.go lines 549-563 to
add assertions that ConditionWorkflowFailed is True and ConditionWorkflowRunning
is False in addition to the existing ConditionWorkflowCompleted assertion.
Update the integration test at controller_integration_test.go lines 199-213
(workflow-not-found test) with the same additional assertions for
ConditionWorkflowFailed=True and ConditionWorkflowRunning=False alongside the
ConditionWorkflowCompleted check.
- Around line 173-179: In the meta.SetStatusCondition call that sets
ConditionWorkflowRunning to False (line 176), change the Reason field from
string(ReasonComponentValidationFailed) to string(ReasonWorkflowRunning) to
match the established pattern used in setWorkflowSucceededCondition,
setWorkflowFailedCondition, and setWorkflowNotFoundCondition. The specific
failure reason ReasonComponentValidationFailed should only be applied to the
ConditionWorkflowFailed and ConditionWorkflowCompleted conditions where it is
already correctly set on lines 183 and 190, not to ConditionWorkflowRunning.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7fac3e81-6222-4521-b571-6a39b44f7567

📥 Commits

Reviewing files that changed from the base of the PR and between c262188 and 8037ecc.

📒 Files selected for processing (1)

internal/controller/workflowrun/controller_conditions.go

…coverage Signed-off-by: kavix <[email protected]>

binoyPeries · 2026-06-17T01:54:51Z

+		Type:               string(ConditionWorkflowFailed),
+		Status:             metav1.ConditionTrue,
+		Reason:             string(ReasonWorkflowFailed),
+		Message:            "Workflow has been deleted from the cluster",


Correct me if I'm wrong, but the main use case here is when a workflow isn't found, right? If so, it's better to mention that in the message. Something like workflow not found would suffice.

You're absolutely right, the main use case is when a workflow isn't found. I've updated the messages for ConditionWorkflowFailed and ConditionWorkflowCompleted in setWorkflowNotFoundCondition to 'Workflow is not found in the cluster' (to keep it consistent with the existing ConditionWorkflowRunning message in that function).

binoyPeries · 2026-06-17T02:00:36Z

Hi @kavix , Thanks for your interest in contributing to OpenChoreo. Can you please fix the failing test cases.

kavix · 2026-06-17T08:10:32Z

Hi @kavix , Thanks for your interest in contributing to OpenChoreo. Can you please fix the failing test cases.

I also updated and fixed the corresponding unit tests, and they are now passing.

…x test Signed-off-by: kavix <[email protected]>

codecov · 2026-06-17T09:40:46Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

LakshanSS

LGTM

LakshanSS · 2026-06-17T14:47:33Z

Thanks for the fix, @kavix!

kavix requested review from ChathurangaKCD, JanakaSandaruwan, LakshanSS, Mirage20, NomadXD, VajiraPrabuddhaka, akila-i, binoyPeries, chalindukodikara, chathuranga95, isala404, mevan-karu, nilushancosta, sameerajayasoma and yashodgayashan as code owners June 16, 2026 20:33

github-actions Bot added the reportedBy/community label Jun 16, 2026

coderabbitai Bot reviewed Jun 16, 2026

View reviewed changes

Comment thread internal/controller/workflowrun/controller_conditions.go

Comment thread internal/controller/workflowrun/controller_conditions.go

fix(workflowrun): align ConditionWorkflowRunning reason and add test …

195fccc

…coverage Signed-off-by: kavix <[email protected]>

kavix force-pushed the fix-workflowrun-pending-state branch from d106efb to 195fccc Compare June 16, 2026 21:43

binoyPeries reviewed Jun 17, 2026

View reviewed changes

fix(workflowrun): update workflow not found condition messages and fi…

44b0e4d

…x test Signed-off-by: kavix <[email protected]>

kavix force-pushed the fix-workflowrun-pending-state branch from 1b2f8ac to 44b0e4d Compare June 17, 2026 08:16

kavix requested a review from binoyPeries June 17, 2026 08:16

LakshanSS approved these changes Jun 17, 2026

View reviewed changes

LakshanSS merged commit 39a1c90 into openchoreo:main Jun 17, 2026
12 of 13 checks passed

Conversation

kavix commented Jun 16, 2026

Uh oh!

coderabbitai Bot commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changed Files & Breakdown

API/CRD Surface Changes

What Changed (controller condition semantics)

Workflow not found (cluster lookup failure)

Component validation failure

Tests (updated)

Integration tests

Unit tests

Risk Hotspots

Walkthrough

Changes

Estimated code review effort

❌ Failed checks (2 warnings)

Review ran into problems

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

binoyPeries Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kavix Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

binoyPeries commented Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kavix commented Jun 17, 2026

Uh oh!

codecov Bot commented Jun 17, 2026

Codecov Report

Uh oh!

LakshanSS left a comment

Choose a reason for hiding this comment

Uh oh!

LakshanSS commented Jun 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai Bot commented Jun 16, 2026 •

edited

Loading

binoyPeries Jun 17, 2026 •

edited

Loading

kavix Jun 17, 2026 •

edited

Loading

binoyPeries commented Jun 17, 2026 •

edited

Loading