(cleanup) use skipping reason instead of existence of variables

today, we existence of variables to determine whether to add `when` expressions to the skipped `tasks` status for finally tasks (we only add them when they are resolved, have no variables) we recently added skipping reason in #4085 now that we have skipping reason, we can use it to check that the reason for skipping is that `when` expressions evaluated to false before including the resolved `when` expressions to the skipped `tasks` status we plan to explore adding skipping reason to the skipped `tasks` status in the future, but for now this change reuses this new functionality
tektoncd · Aug 17, 2021 · adf1953 · adf1953
1 parent 89a6233
commit adf1953
Show file tree

Hide file tree

Showing 4 changed files with 54 additions and 102 deletions.
diff --git a/pkg/apis/pipeline/v1beta1/when_types.go b/pkg/apis/pipeline/v1beta1/when_types.go
@@ -54,13 +54,6 @@ func (we *WhenExpression) isTrue() bool {
 	return !we.isInputInValues()
 }
 
-func (we *WhenExpression) hasVariable() bool {
-	if _, hasVariable := we.GetVarSubstitutionExpressions(); hasVariable {
-		return true
-	}
-	return false
-}
-
 func (we *WhenExpression) applyReplacements(replacements map[string]string, arrayReplacements map[string][]string) WhenExpression {
 	replacedInput := substitution.ApplyReplacements(we.Input, replacements)
 
@@ -105,17 +98,6 @@ func (wes WhenExpressions) AllowsExecution() bool {
 	return true
 }
 
-// HaveVariables indicates whether When Expressions contains variables, such as Parameters
-// or Results in the Inputs or Values.
-func (wes WhenExpressions) HaveVariables() bool {
-	for _, we := range wes {
-		if we.hasVariable() {
-			return true
-		}
-	}
-	return false
-}
-
 // ReplaceWhenExpressionsVariables interpolates variables, such as Parameters and Results, in
 // the Input and Values.
 func (wes WhenExpressions) ReplaceWhenExpressionsVariables(replacements map[string]string, arrayReplacements map[string][]string) WhenExpressions {

diff --git a/pkg/apis/pipeline/v1beta1/when_types_test.go b/pkg/apis/pipeline/v1beta1/when_types_test.go
@@ -88,66 +88,6 @@ func TestAllowsExecution(t *testing.T) {
 	}
 }
 
-func TestHaveVariables(t *testing.T) {
-	tests := []struct {
-		name            string
-		whenExpressions WhenExpressions
-		expected        bool
-	}{{
-		name: "doesn't have variable",
-		whenExpressions: WhenExpressions{
-			{
-				Input:    "foo",
-				Operator: selection.In,
-				Values:   []string{"foo", "bar"},
-			},
-		},
-		expected: false,
-	}, {
-		name: "have variable - input",
-		whenExpressions: WhenExpressions{
-			{
-				Input:    "$(params.foobar)",
-				Operator: selection.NotIn,
-				Values:   []string{"foobar"},
-			},
-		},
-		expected: true,
-	}, {
-		name: "have variable - values",
-		whenExpressions: WhenExpressions{
-			{
-				Input:    "foobar",
-				Operator: selection.In,
-				Values:   []string{"$(params.foobar)"},
-			},
-		},
-		expected: true,
-	}, {
-		name: "have variable - multiple when expressions",
-		whenExpressions: WhenExpressions{
-			{
-				Input:    "foo",
-				Operator: selection.NotIn,
-				Values:   []string{"bar"},
-			}, {
-				Input:    "foobar",
-				Operator: selection.In,
-				Values:   []string{"$(params.foobar)"},
-			},
-		},
-		expected: true,
-	}}
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			got := tc.whenExpressions.HaveVariables()
-			if d := cmp.Diff(tc.expected, got); d != "" {
-				t.Errorf("Error evaluating HaveVariables() for When Expressions in test case %s", diff.PrintWantGot(d))
-			}
-		})
-	}
-}
-
 func TestReplaceWhenExpressionsVariables(t *testing.T) {
 	tests := []struct {
 		name            string

diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go b/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go
@@ -423,7 +423,7 @@ func (facts *PipelineRunFacts) GetSkippedTasks() []v1beta1.SkippedTask {
 			}
 			// include the when expressions only when the finally task was skipped because
 			// its when expressions evaluated to false (not because results variables were missing)
-			if !rprt.PipelineTask.WhenExpressions.HaveVariables() {
+			if rprt.IsFinallySkipped(facts).SkippingReason == WhenExpressionsSkip {
 				skippedTask.WhenExpressions = rprt.PipelineTask.WhenExpressions
 			}
 			skipped = append(skipped, skippedTask)

diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go b/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go
@@ -1586,30 +1586,60 @@ func TestPipelineRunFacts_GetPipelineTaskStatus(t *testing.T) {
 }
 
 func TestPipelineRunFacts_GetSkippedTasks(t *testing.T) {
-	dagFailedFinalSkipped := PipelineRunState{{
-		TaskRunName:  "task0taskrun",
-		PipelineTask: &pts[0],
-		TaskRun:      makeFailed(trs[0]),
+	for _, tc := range []struct {
+		name                 string
+		state                PipelineRunState
+		dagTasks             []v1beta1.PipelineTask
+		finallyTasks         []v1beta1.PipelineTask
+		expectedSkippedTasks []v1beta1.SkippedTask
+	}{{
+		name: "missing-results-skip-finally",
+		state: PipelineRunState{{
+			TaskRunName:  "task0taskrun",
+			PipelineTask: &pts[0],
+			TaskRun:      makeFailed(trs[0]),
+		}, {
+			PipelineTask: &pts[14],
+		}},
+		dagTasks:     []v1beta1.PipelineTask{pts[0]},
+		finallyTasks: []v1beta1.PipelineTask{pts[14]},
+		expectedSkippedTasks: []v1beta1.SkippedTask{{
+			Name: pts[14].Name,
+		}},
 	}, {
-		PipelineTask: &pts[14],
-	}}
-	expectedSkippedTasks := []v1beta1.SkippedTask{{Name: pts[14].Name}}
-	d, err := dag.Build(v1beta1.PipelineTaskList{pts[0]}, v1beta1.PipelineTaskList{pts[0]}.Deps())
-	if err != nil {
-		t.Fatalf("Unexpected error while building graph for DAG tasks %v: %v", v1beta1.PipelineTaskList{pts[0]}, err)
-	}
-	df, err := dag.Build(v1beta1.PipelineTaskList{pts[14]}, map[string][]string{})
-	if err != nil {
-		t.Fatalf("Unexpected error while building graph for final tasks %v: %v", v1beta1.PipelineTaskList{pts[14]}, err)
-	}
-	facts := PipelineRunFacts{
-		State:           dagFailedFinalSkipped,
-		TasksGraph:      d,
-		FinalTasksGraph: df,
-	}
-	actualSkippedTasks := facts.GetSkippedTasks()
-	if d := cmp.Diff(actualSkippedTasks, expectedSkippedTasks); d != "" {
-		t.Fatalf("Mismatch skipped tasks %s", diff.PrintWantGot(d))
+		name: "when-expressions-skip-finally",
+		state: PipelineRunState{{
+			PipelineTask: &pts[10],
+		}},
+		finallyTasks: []v1beta1.PipelineTask{pts[10]},
+		expectedSkippedTasks: []v1beta1.SkippedTask{{
+			Name: pts[10].Name,
+			WhenExpressions: []v1beta1.WhenExpression{{
+				Input:    "foo",
+				Operator: "notin",
+				Values:   []string{"foo", "bar"},
+			}},
+		}},
+	}} {
+		t.Run(tc.name, func(t *testing.T) {
+			d, err := dag.Build(v1beta1.PipelineTaskList(tc.dagTasks), v1beta1.PipelineTaskList(tc.dagTasks).Deps())
+			if err != nil {
+				t.Fatalf("Unexpected error while building graph for DAG tasks %v: %v", v1beta1.PipelineTaskList{pts[0]}, err)
+			}
+			df, err := dag.Build(v1beta1.PipelineTaskList(tc.finallyTasks), map[string][]string{})
+			if err != nil {
+				t.Fatalf("Unexpected error while building graph for final tasks %v: %v", v1beta1.PipelineTaskList{pts[14]}, err)
+			}
+			facts := PipelineRunFacts{
+				State:           tc.state,
+				TasksGraph:      d,
+				FinalTasksGraph: df,
+			}
+			actualSkippedTasks := facts.GetSkippedTasks()
+			if d := cmp.Diff(tc.expectedSkippedTasks, actualSkippedTasks); d != "" {
+				t.Fatalf("Mismatch skipped tasks %s", diff.PrintWantGot(d))
+			}
+		})
 	}
 }