Skip to content

Commit

Permalink
Prior to this PR, when the when.cel field in the final task refers …
Browse files Browse the repository at this point in the history
…to ordinary task status,

it cannot take effect. In fact, when cel is calculated, the expression is not replaced,
and the original literal is used, such as: `"'$(tasks.a-task. status)' == 'Succeeded'"`.

This commit will be ensured that the status of the referenced ordinary task is replaced
before calculating the final task `when.cel`.
  • Loading branch information
cugykw authored and tekton-robot committed Feb 12, 2024
1 parent 2c11092 commit 1568ed1
Show file tree
Hide file tree
Showing 3 changed files with 178 additions and 3 deletions.
8 changes: 8 additions & 0 deletions pkg/reconciler/pipelinerun/pipelinerun.go
Original file line number Diff line number Diff line change
Expand Up @@ -860,6 +860,14 @@ func (c *Reconciler) runNextSchedulableTask(ctx context.Context, pr *v1.Pipeline
continue
}
resources.ApplyTaskResults(resources.PipelineRunState{rpt}, resolvedResultRefs)

if err := rpt.EvaluateCEL(); err != nil {
logger.Errorf("Final task %q is not executed, due to error evaluating CEL %s: %v", rpt.PipelineTask.Name, pr.Name, err)
pr.Status.MarkFailed(string(v1.PipelineRunReasonCELEvaluationFailed),
"Error evaluating CEL %s: %w", pr.Name, pipelineErrors.WrapUserError(err))
return controller.NewPermanentError(err)
}

nextRpts = append(nextRpts, rpt)
}
}
Expand Down
168 changes: 168 additions & 0 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4643,6 +4643,174 @@ spec:
}
}

func TestReconcileWithFinalTasksCELWhenExpressions(t *testing.T) {
ps := []*v1.Pipeline{parse.MustParseV1Pipeline(t, `
metadata:
name: test-pipeline
namespace: foo
spec:
params:
- name: run
type: string
tasks:
- name: a-task
taskRef:
name: a-task
- name: b-task
taskRef:
name: b-task
finally:
- name: f-c-task
taskRef:
name: f-c-task
when:
- cel: "'$(tasks.a-task.results.aResult)' == 'aResultValue'"
- cel: "'$(params.run)'=='yes'"
- cel: "'$(tasks.a-task.status)' == 'Succeeded'"
- name: f-d-task
taskRef:
name: f-d-task
when:
- cel: "'$(tasks.b-task.status)' == 'Succeeded'"
`)}
prs := []*v1.PipelineRun{parse.MustParseV1PipelineRun(t, `
metadata:
name: test-pipeline-run-different-final-task-when
namespace: foo
spec:
params:
- name: run
value: "yes"
pipelineRef:
name: test-pipeline
taskRunTemplate:
serviceAccountName: test-sa-0
`)}
ts := []*v1.Task{
{ObjectMeta: baseObjectMeta("a-task", "foo")},
{ObjectMeta: baseObjectMeta("b-task", "foo")},
{ObjectMeta: baseObjectMeta("f-c-task", "foo")},
{ObjectMeta: baseObjectMeta("f-d-task", "foo")},
}
trs := []*v1.TaskRun{mustParseTaskRunWithObjectMeta(t,
taskRunObjectMeta("test-pipeline-run-different-final-task-when-a-task-xxyyy", "foo", "test-pipeline-run-different-final-task-when",
"test-pipeline", "a-task", true),
`
spec:
serviceAccountName: test-sa
taskRef:
name: hello-world
timeout: 1h0m0s
status:
conditions:
- status: "True"
type: Succeeded
results:
- name: aResult
value: aResultValue
`), mustParseTaskRunWithObjectMeta(t,
taskRunObjectMeta("test-pipeline-run-different-final-task-when-b-task-xxyyy", "foo", "test-pipeline-run-different-final-task-when",
"test-pipeline", "b-task", true),
`
spec:
serviceAccountName: test-sa
taskRef:
name: hello-world
timeout: 1h0m0s
status:
conditions:
- status: "False"
type: Succeeded
`)}
cms := []*corev1.ConfigMap{
{
ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.Namespace()},
Data: map[string]string{
"enable-cel-in-whenexpression": "true",
},
},
}
d := test.Data{
PipelineRuns: prs,
Pipelines: ps,
Tasks: ts,
TaskRuns: trs,
ConfigMaps: cms,
}
prt := newPipelineRunTest(t, d)
defer prt.Cancel()

wantEvents := []string{
"Normal Started",
"Normal Running Tasks Completed: 2 \\(Failed: 1, Cancelled 0\\), Incomplete: 1, Skipped: 1",
}
pipelineRun, clients := prt.reconcileRun("foo", "test-pipeline-run-different-final-task-when", wantEvents, false)

expectedTaskRunName := "test-pipeline-run-different-final-task-when-f-c-task"
expectedTaskRun := mustParseTaskRunWithObjectMeta(t,
taskRunObjectMeta(expectedTaskRunName, "foo", "test-pipeline-run-different-final-task-when", "test-pipeline", "f-c-task", true),
`
spec:
serviceAccountName: test-sa-0
taskRef:
name: f-c-task
kind: Task
`)
expectedTaskRun.Labels[pipeline.MemberOfLabelKey] = v1.PipelineFinallyTasks
// Check that the expected TaskRun was created
actual, err := clients.Pipeline.TektonV1().TaskRuns("foo").List(prt.TestAssets.Ctx, metav1.ListOptions{
LabelSelector: "tekton.dev/pipelineTask=f-c-task,tekton.dev/pipelineRun=test-pipeline-run-different-final-task-when",
Limit: 1,
})

if err != nil {
t.Fatalf("Failure to list TaskRun's %s", err)
}
if len(actual.Items) != 1 {
t.Fatalf("Expected 1 TaskRuns got %d", len(actual.Items))
}
actualTaskRun := actual.Items[0]
if d := cmp.Diff(expectedTaskRun, &actualTaskRun, ignoreResourceVersion, ignoreTypeMeta); d != "" {
t.Errorf("expected to see TaskRun %v created. Diff %s", expectedTaskRunName, diff.PrintWantGot(d))
}

expectedWhenExpressionsInTaskRun := []v1.WhenExpression{{
CEL: "'aResultValue' == 'aResultValue'",
}, {
CEL: "'yes'=='yes'",
}, {
CEL: "'Succeeded' == 'Succeeded'",
}}
verifyTaskRunStatusesWhenExpressions(t, pipelineRun.Status, expectedTaskRunName, expectedWhenExpressionsInTaskRun)

actualSkippedTasks := pipelineRun.Status.SkippedTasks
expectedSkippedTasks := []v1.SkippedTask{{
Name: "f-d-task",
Reason: v1.WhenExpressionsSkip,
WhenExpressions: v1.WhenExpressions{{
CEL: "'Failed' == 'Succeeded'",
}},
}}
if d := cmp.Diff(expectedSkippedTasks, actualSkippedTasks); d != "" {
t.Errorf("expected to find Skipped Tasks %v. Diff %s", expectedSkippedTasks, diff.PrintWantGot(d))
}

skippedTasks := []string{"f-d-task"}
for _, skippedTask := range skippedTasks {
labelSelector := fmt.Sprintf("tekton.dev/pipelineTask=%s,tekton.dev/pipelineRun=test-pipeline-run-different-final-task-when", skippedTask)
actualSkippedTask, err := clients.Pipeline.TektonV1().TaskRuns("foo").List(prt.TestAssets.Ctx, metav1.ListOptions{
LabelSelector: labelSelector,
Limit: 1,
})
if err != nil {
t.Fatalf("Failure to list TaskRun's %s", err)
}
if len(actualSkippedTask.Items) != 0 {
t.Fatalf("Expected 0 TaskRuns got %d", len(actualSkippedTask.Items))
}
}
}

func TestReconcile_InvalidCELWhenExpressions(t *testing.T) {
ps := []*v1.Pipeline{parse.MustParseV1Pipeline(t, `
metadata:
Expand Down
5 changes: 2 additions & 3 deletions pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,8 @@ type ResolvedPipelineTask struct {
// EvaluateCEL evaluate the CEL expressions, and store the evaluated results in EvaluatedCEL
func (t *ResolvedPipelineTask) EvaluateCEL() error {
if t.PipelineTask != nil {
if len(t.EvaluatedCEL) == 0 {
t.EvaluatedCEL = make(map[string]bool)
}
// Each call to this function will reset this field to prevent additional CELs.
t.EvaluatedCEL = make(map[string]bool)
for _, we := range t.PipelineTask.When {
if we.CEL == "" {
continue
Expand Down

0 comments on commit 1568ed1

Please sign in to comment.