From b7300ae0c7e1e75e8ce776bfe52366d9eb0d8d63 Mon Sep 17 00:00:00 2001 From: Jerop Date: Thu, 10 Sep 2020 12:55:03 -0400 Subject: [PATCH] Add check before creating task Tasks with when expressions using variables that evaluated to false were mistakenly executed because it missed an additional check to validate that the task should be skipped after variable replacement. Fixes #3188 (cherry picked from commit c12b84311e6fa5b7bc9c3d005c705c9944909be3) --- .../pipelinerun-with-when-expressions.yaml | 10 +++++++ pkg/reconciler/pipelinerun/pipelinerun.go | 2 +- .../pipelinerun/pipelinerun_test.go | 30 +++++++++++++++++++ 3 files changed, 41 insertions(+), 1 deletion(-) diff --git a/examples/v1beta1/pipelineruns/pipelinerun-with-when-expressions.yaml b/examples/v1beta1/pipelineruns/pipelinerun-with-when-expressions.yaml index a2a399f15c0..8ce3b2862da 100644 --- a/examples/v1beta1/pipelineruns/pipelinerun-with-when-expressions.yaml +++ b/examples/v1beta1/pipelineruns/pipelinerun-with-when-expressions.yaml @@ -66,6 +66,16 @@ spec: - name: echo image: ubuntu script: 'echo file exists' + - name: echo-file-missing + when: + - input: "$(tasks.check-file.results.exists)" + operator: in + values: ["missing"] + taskSpec: + steps: + - name: echo + image: ubuntu + script: 'echo file missing' - name: task-should-be-skipped when: - input: "$(params.path)" diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index fcdafc03ead..04e27409920 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -522,7 +522,7 @@ func (c *Reconciler) runNextSchedulableTask(ctx context.Context, pr *v1beta1.Pip resources.ApplyTaskResults(nextRprts, resolvedResultRefs) for _, rprt := range nextRprts { - if rprt == nil { + if rprt == nil || rprt.Skip(pipelineState, d) { continue } if rprt.ResolvedConditionChecks == nil || rprt.ResolvedConditionChecks.IsSuccess() { diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 25ccb7e1a54..cd9262d45a4 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -2081,6 +2081,21 @@ func TestReconcileWithWhenExpressionsWithParameters(t *testing.T) { if d := cmp.Diff(actualSkippedTasks, expectedSkippedTasks); d != "" { t.Errorf("expected to find Skipped Tasks %v. Diff %s", expectedSkippedTasks, diff.PrintWantGot(d)) } + + skippedTasks := []string{"hello-world-2"} + for _, skippedTask := range skippedTasks { + labelSelector := fmt.Sprintf("tekton.dev/pipelineTask=%s,tekton.dev/pipelineRun=test-pipeline-run-different-service-accs", skippedTask) + actualSkippedTask, err := clients.Pipeline.TektonV1beta1().TaskRuns("foo").List(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 TestReconcileWithWhenExpressionsWithTaskResults(t *testing.T) { @@ -2189,6 +2204,21 @@ func TestReconcileWithWhenExpressionsWithTaskResults(t *testing.T) { if d := cmp.Diff(actualSkippedTasks, expectedSkippedTasks); d != "" { t.Errorf("expected to find Skipped Tasks %v. Diff %s", expectedSkippedTasks, diff.PrintWantGot(d)) } + + skippedTasks := []string{"c-task", "d-task"} + for _, skippedTask := range skippedTasks { + labelSelector := fmt.Sprintf("tekton.dev/pipelineTask=%s,tekton.dev/pipelineRun=test-pipeline-run-different-service-accs", skippedTask) + actualSkippedTask, err := clients.Pipeline.TektonV1beta1().TaskRuns("foo").List(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)) + } + } } // TestReconcileWithAffinityAssistantStatefulSet tests that given a pipelineRun with workspaces,