Skip to content

Commit

Permalink
Add check before creating task
Browse files Browse the repository at this point in the history
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 tektoncd#3188

(cherry picked from commit c12b843)
  • Loading branch information
jerop authored and bobcatfish committed Sep 14, 2020
1 parent a162a1d commit b7300ae
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -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)"
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/pipelinerun/pipelinerun.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
30 changes: 30 additions & 0 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand Down

0 comments on commit b7300ae

Please sign in to comment.