From adf1953542467da17e189f3fb7b7b1a3a3a2050c Mon Sep 17 00:00:00 2001 From: Jerop 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 +- .../resources/pipelinerunstate_test.go | 76 +++++++++++++------ 4 files changed, 54 insertions(+), 102 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) diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go b/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go index 0ca19c57774..766b73b548b 100644 --- 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)) + } + }) } }