From f0e4efd84b8426a287ab642159d2553160dca5ae Mon Sep 17 00:00:00 2001
From: Jerop <jerop@google.com>
Date: Wed, 11 Aug 2021 11:10:18 -0400
Subject: [PATCH] (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 https://github.com/tektoncd/pipeline/pull/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
---
 pkg/apis/pipeline/v1beta1/when_types.go       | 18 ------
 pkg/apis/pipeline/v1beta1/when_types_test.go  | 60 -------------------
 .../pipelinerun/resources/pipelinerunstate.go |  2 +-
 3 files changed, 1 insertion(+), 79 deletions(-)

diff --git a/pkg/apis/pipeline/v1beta1/when_types.go b/pkg/apis/pipeline/v1beta1/when_types.go
index 08ac1a8f8a5..929ae8b438d 100644
--- 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
index 439efcc1e3a..5f9dbbe195e 100644
--- 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
index 078b52fe262..e95c291b1ca 100644
--- 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)