PR #274 - kubevela/workflow

Description of your changes

Three fixes found while writing the 1.12 docs, one commit (or one short run of commits) per fix, so each can be read, reverted or backported on its own.

Fix	Commits	Released?
A skipped step with a path output panics the reconcile	1	Yes: v0.7.2 (KubeVela v1.11.0-alpha.6)
A failed item in a DAG `forEach` skips the items after it	1	No: `forEach` is unreleased
A `$( )` condition is skipped silently where it cannot be evaluated	4	No: `$( )` conditions are unreleased

1. A skipped step with a path output no longer panics the reconcile

A step skipped by its if, with an output such as valueFrom: parameter.message, panics the controller on every reconcile, so the run never gets past it:

steps:
  - name: producer
    type: echo
    if: "false"
    properties: {msg: hi}
    outputs: [{name: x, valueFrom: parameter.msg}]
  - name: after
    type: echo
    properties: {msg: done}

A skipped step is never rendered, so its post-step hooks get a stand-in value, and that stand-in is a CUE conflict. The output hook looks each valueFrom up in it, and CUE panics on a path lookup in an error value (not properly initialized) rather than returning an error. The step's recover sits above the hooks in the same deferred function, so nothing catches it.

LookupValueByScript now returns the value's own error before looking anything up. The output hook then records a skipped step's outputs as null, as it already means to, and the run carries on. A literal valueFrom was never affected.

2. A failed item in a DAG forEach no longer skips the items after it

In DAG mode an item's failure is meant to affect only that item, and within one reconcile it did. But the shortcut that leaves an already finished item alone also carried its failure forward in both modes. So on a later reconcile, an item still running after a failed one was rebuilt as a skip: whether a DAG item ran depended on reconcile timing. The shortcut now carries a failure forward only in StepByStep, where a failed item does skip the items after it.

3. A `$( )` condition is no longer skipped silently

A step whose if is a $( ) condition, such as if: $(steps.a.succeeded), never runs in a WorkflowRun that does not read expressions (the EnableCelExpressions gate off, or no workflowrun.oam.dev/cel-expressions: "true" annotation). Admission let it through. With no evaluator, the engine read the if as CUE, which it is not, failed to compile it, and skipped the step. The run then reported succeeded, with only a step message as a clue. An if holding a $( ) that is not the whole condition, such as $(a) && $(b), met the same end whether or not expressions were on.

This closes both, at admission and at run time:

Admission (the WorkflowRun webhook) refuses a $( ) condition in a run that does not read expressions, on a step or a sub-step, naming the switch that is off.
It also refuses an if holding a $( ) outside its CUE strings and comments that is not one expression, with expressions on or off.
Both checks are exported from pkg/expressions (ConditionsWithoutExpressions, MisplacedConditions) for KubeVela's Application webhook.
Run time: the pre-check now fails such a step, with a message saying why, instead of skipping it. This covers a run admitted without the check (the webhook disabled, or admitted before this change).
PreCheckResult gains Fail; a step, a step group and a loop each fail on it and run nothing further.

Any other if is CUE and behaves as before, including a $( ) inside a CUE string.

I have:

Read and followed KubeVela's contribution process.
Related Docs updated properly. In a new feature or configuration option, an update to the documentation is necessary.
Run make reviewable to ensure this PR is ready for review.
Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Every fix has a test that failed before its change.

Fix 1:
- Tests at three layers: LookupValueByScript on an error value (an error, not a panic); the output hook for a skipped step (records null, no error); a generator spec running the YAML above (producer skipped, after succeeded, x is null). Without the guard, the spec panics at the same CUE frame as the controller does.
- Confirmed affected on v0.7.2 directly.
Fix 2: two-reconcile tests for both modes.
- In DAG, item b keeps running after item a failed on an earlier reconcile.
- In StepByStep, a failure recorded on an earlier reconcile still skips b.
- Weakening either half of the change fails its test.
Live, fixes 1 and 2: the standalone controller on a throwaway k3d, built from this branch and from main, with the same WorkflowRuns.
- Fix 1 on main: 14 panics in 75 seconds, and the run never recorded a step status. With the fix: producer skipped, after succeeded, x: null, no panics.
- Fix 2 on main: item b is skipped once a fails. With the fix: b keeps running until its own work finishes, then succeeds.
Fix 3:
- Unit tests for both checks (steps, sub-steps, CUE strings, escapes, comments), the WorkflowRun webhook (gate off, not opted in, opted in, sub-steps, two expressions), the pre-check, and the custom, step-group and loop runners on Fail.
- Live, through KubeVela built against this branch: an opted-in $( ) if still runs on a plain step, a step-group sub-step and both iterations of a forEach group. Without the opt-in, all three are refused, and $(a) && $(b) is refused.
The whole branch:
- go test passes in every package except pkg/providers/email and pkg/providers/legacy/email. There, TestSendEmail fails identically on unchanged main when run on macOS arm64: gomonkey's binary patching is refused (permission denied). Linux CI is unaffected.
- make reviewable: golangci-lint and staticcheck are clean. It also regenerates three files this branch does not touch (the deepcopy header, the WorkflowStepDefinition CRD synced from KubeVela master, and the chart README's table widths). That drift is in main too, so I've left it out.

Special notes for your reviewer

Fix 1 is in a release, so it is the one to backport: workflow v0.7.2, which KubeVela v1.11.0-alpha.6 pins. Its commit stands alone. On v0.7.2, the code change and its value and hook tests apply cleanly and pass; only the generator spec conflicts with neighbouring specs there and needs placing by hand. Fixes 2 and 3 cover features that have not been tagged yet.

The companion KubeVela PR adds the same admission checks for Applications.

Summary by cubic

Fixes three workflow bugs: skipped steps with path outputs no longer panic the reconcile, DAG forEach items keep running after an earlier item fails, and $( ) conditions that cannot be evaluated are failed instead of silently skipped.

Bug Fixes

A skipped step's path output is recorded as null instead of panicking in CUE; this fix is released in workflow v0.7.2 and is the one to backport.
A failed item in a DAG forEach no longer skips later items; StepByStep still skips them.
A $( ) condition the run cannot evaluate (gate off, not opted in) is refused at admission and failed at run time instead of silently skipped.
Admission also refuses an if where $( ) is not the whole condition; the checks are exported for KubeVela's Application webhook.