From acfeaad5d09519382225ffa7d2ae56bef2a55f41 Mon Sep 17 00:00:00 2001 From: Christie Wilson Date: Wed, 22 Apr 2020 18:31:38 -0400 Subject: [PATCH] =?UTF-8?q?Determine=20correct=20graph=20when=20params=20a?= =?UTF-8?q?nd=20results=20are=20mixed=20=F0=9F=A5=84?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This was partially addressed in #2387 however the example from the bug (#2387) was using runAfter, and it should be possible to determine the correct graph without runAfter. The logic that was determining the graph was returning an error if any of the substitutions present in a string along with a result were not results. Now we will instead ignore these substitutions. We might want to revisit the validation to make sure that invalid results substitutions are still caught. (cherry picked from commit 4b1b76167fc60a4468cae179329c7260e727ea02) Updated test logic that used builder which were added / changed in other commits on master. --- .../pipelinerun-results-with-params.yaml | 4 +- pkg/apis/pipeline/v1alpha1/pipeline_types.go | 16 +++- pkg/apis/pipeline/v1beta1/pipeline_types.go | 14 ++-- .../pipeline/v1beta1/pipeline_validation.go | 5 +- pkg/apis/pipeline/v1beta1/resultref.go | 21 ++--- pkg/apis/pipeline/v1beta1/resultref_test.go | 76 +++++++++++-------- .../pipelinerun/pipelinerun_test.go | 74 ++++++++++++++++++ .../resources/resultrefresolution.go | 18 ++--- 8 files changed, 161 insertions(+), 67 deletions(-) diff --git a/examples/v1beta1/pipelineruns/pipelinerun-results-with-params.yaml b/examples/v1beta1/pipelineruns/pipelinerun-results-with-params.yaml index 6fcf30307a1..d584f904bde 100644 --- a/examples/v1beta1/pipelineruns/pipelinerun-results-with-params.yaml +++ b/examples/v1beta1/pipelineruns/pipelinerun-results-with-params.yaml @@ -21,8 +21,6 @@ spec: script: | echo -n "suffix" > $(results.suffix.path) - name: do-something - runAfter: - - generate-suffix taskSpec: params: - name: arg @@ -30,7 +28,7 @@ spec: - name: do-something image: alpine script: | - echo "$(params.arg)" + echo "$(params.arg)" | grep "prefix:suffix" params: - name: arg value: "$(params.prefix):$(tasks.generate-suffix.results.suffix)" diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_types.go b/pkg/apis/pipeline/v1alpha1/pipeline_types.go index 2ef98a06dd1..ae0ae3332f7 100644 --- a/pkg/apis/pipeline/v1alpha1/pipeline_types.go +++ b/pkg/apis/pipeline/v1alpha1/pipeline_types.go @@ -161,15 +161,23 @@ func (pt PipelineTask) Deps() []string { for _, rd := range cond.Resources { deps = append(deps, rd.From...) } + for _, param := range cond.Params { + expressions, ok := v1beta1.GetVarSubstitutionExpressionsForParam(param) + if ok { + resultRefs := v1beta1.NewResultRefs(expressions) + for _, resultRef := range resultRefs { + deps = append(deps, resultRef.PipelineTask) + } + } + } } // Add any dependents from task results for _, param := range pt.Params { expressions, ok := v1beta1.GetVarSubstitutionExpressionsForParam(param) if ok { - if resultRefs, err := v1beta1.NewResultRefs(expressions); err == nil { - for _, resultRef := range resultRefs { - deps = append(deps, resultRef.PipelineTask) - } + resultRefs := v1beta1.NewResultRefs(expressions) + for _, resultRef := range resultRefs { + deps = append(deps, resultRef.PipelineTask) } } } diff --git a/pkg/apis/pipeline/v1beta1/pipeline_types.go b/pkg/apis/pipeline/v1beta1/pipeline_types.go index ba1bf433ac4..28a977ff1a6 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_types.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_types.go @@ -139,10 +139,9 @@ func (pt PipelineTask) Deps() []string { for _, param := range cond.Params { expressions, ok := GetVarSubstitutionExpressionsForParam(param) if ok { - if resultRefs, err := NewResultRefs(expressions); err == nil { - for _, resultRef := range resultRefs { - deps = append(deps, resultRef.PipelineTask) - } + resultRefs := NewResultRefs(expressions) + for _, resultRef := range resultRefs { + deps = append(deps, resultRef.PipelineTask) } } } @@ -151,10 +150,9 @@ func (pt PipelineTask) Deps() []string { for _, param := range pt.Params { expressions, ok := GetVarSubstitutionExpressionsForParam(param) if ok { - if resultRefs, err := NewResultRefs(expressions); err == nil { - for _, resultRef := range resultRefs { - deps = append(deps, resultRef.PipelineTask) - } + resultRefs := NewResultRefs(expressions) + for _, resultRef := range resultRefs { + deps = append(deps, resultRef.PipelineTask) } } } diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation.go b/pkg/apis/pipeline/v1beta1/pipeline_validation.go index c5bb9ef576f..f26fa336027 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_validation.go @@ -326,8 +326,9 @@ func validateParamResults(tasks []PipelineTask) error { if ok { if LooksLikeContainsResultRefs(expressions) { expressions = filter(expressions, looksLikeResultRef) - if _, err := NewResultRefs(expressions); err != nil { - return err + resultRefs := NewResultRefs(expressions) + if len(expressions) != len(resultRefs) { + return fmt.Errorf("expected all of the expressions %v to be result expressions but only %v were", expressions, resultRefs) } } } diff --git a/pkg/apis/pipeline/v1beta1/resultref.go b/pkg/apis/pipeline/v1beta1/resultref.go index 1c07e8a69ab..969a232fbfa 100644 --- a/pkg/apis/pipeline/v1beta1/resultref.go +++ b/pkg/apis/pipeline/v1beta1/resultref.go @@ -38,20 +38,23 @@ const ( var variableSubstitutionRegex = regexp.MustCompile(variableSubstitutionFormat) // NewResultRefs extracts all ResultReferences from a param or a pipeline result. -// If the ResultReference can be extracted, they are returned. Otherwise an error is returned -func NewResultRefs(expressions []string) ([]*ResultRef, error) { +// If the ResultReference can be extracted, they are returned. Expressions which are not +// results are ignored. +func NewResultRefs(expressions []string) []*ResultRef { var resultRefs []*ResultRef for _, expression := range expressions { pipelineTask, result, err := parseExpression(expression) - if err != nil { - return nil, fmt.Errorf("Invalid result reference expression: %v", err) + // If the expression isn't a result but is some other expression, + // parseExpression will return an error, in which case we just skip that expression, + // since although it's not a result ref, it might be some other kind of reference + if err == nil { + resultRefs = append(resultRefs, &ResultRef{ + PipelineTask: pipelineTask, + Result: result, + }) } - resultRefs = append(resultRefs, &ResultRef{ - PipelineTask: pipelineTask, - Result: result, - }) } - return resultRefs, nil + return resultRefs } // LooksLikeContainsResultRefs attempts to check if param or a pipeline result looks like it contains any diff --git a/pkg/apis/pipeline/v1beta1/resultref_test.go b/pkg/apis/pipeline/v1beta1/resultref_test.go index 305bbb83da2..4486b00a1df 100644 --- a/pkg/apis/pipeline/v1beta1/resultref_test.go +++ b/pkg/apis/pipeline/v1beta1/resultref_test.go @@ -26,10 +26,9 @@ func TestNewResultReference(t *testing.T) { param v1beta1.Param } tests := []struct { - name string - args args - want []*v1beta1.ResultRef - wantErr bool + name string + args args + want []*v1beta1.ResultRef }{ { name: "Test valid expression", @@ -48,9 +47,8 @@ func TestNewResultReference(t *testing.T) { Result: "sumResult", }, }, - wantErr: false, }, { - name: "Test valid expression: substitution within string", + name: "substitution within string", args: args{ param: v1beta1.Param{ Name: "param", @@ -66,9 +64,8 @@ func TestNewResultReference(t *testing.T) { Result: "sumResult", }, }, - wantErr: false, }, { - name: "Test valid expression: multiple substitution", + name: "multiple substitution", args: args{ param: v1beta1.Param{ Name: "param", @@ -87,9 +84,28 @@ func TestNewResultReference(t *testing.T) { Result: "sumResult", }, }, - wantErr: false, }, { - name: "Test invalid expression: first separator typo", + name: "multiple substitution with param", + args: args{ + param: v1beta1.Param{ + Name: "param", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeString, + StringVal: "$(params.param) $(tasks.sumTask1.results.sumResult) and another $(tasks.sumTask2.results.sumResult)", + }, + }, + }, + want: []*v1beta1.ResultRef{ + { + PipelineTask: "sumTask1", + Result: "sumResult", + }, { + PipelineTask: "sumTask2", + Result: "sumResult", + }, + }, + }, { + name: "first separator typo", args: args{ param: v1beta1.Param{ Name: "param", @@ -99,10 +115,9 @@ func TestNewResultReference(t *testing.T) { }, }, }, - want: nil, - wantErr: true, + want: nil, }, { - name: "Test invalid expression: third separator typo", + name: "third separator typo", args: args{ param: v1beta1.Param{ Name: "param", @@ -112,10 +127,9 @@ func TestNewResultReference(t *testing.T) { }, }, }, - want: nil, - wantErr: true, + want: nil, }, { - name: "Test invalid expression: param substitution shouldn't be considered result ref", + name: "param substitution shouldn't be considered result ref", args: args{ param: v1beta1.Param{ Name: "param", @@ -125,10 +139,9 @@ func TestNewResultReference(t *testing.T) { }, }, }, - want: nil, - wantErr: true, + want: nil, }, { - name: "Test invalid expression: One bad and good result substitution", + name: "One bad and good result substitution", args: args{ param: v1beta1.Param{ Name: "param", @@ -138,23 +151,24 @@ func TestNewResultReference(t *testing.T) { }, }, }, - want: nil, - wantErr: true, + want: []*v1beta1.ResultRef{ + { + PipelineTask: "sumTask1", + Result: "sumResult", + }, + }, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { expressions, ok := v1beta1.GetVarSubstitutionExpressionsForParam(tt.args.param) - if !ok { + if !ok && tt.want != nil { t.Fatalf("expected to find expressions but didn't find any") - } - got, err := v1beta1.NewResultRefs(expressions) - if tt.wantErr != (err != nil) { - t.Errorf("TestNewResultReference/%s wantErr %v, error = %v", tt.name, tt.wantErr, err) - return - } - if d := cmp.Diff(tt.want, got); d != "" { - t.Errorf("TestNewResultReference/%s (-want, +got) = %v", tt.name, d) + } else { + got := v1beta1.NewResultRefs(expressions) + if d := cmp.Diff(tt.want, got); d != "" { + t.Errorf("TestNewResultReference/%s (-want, +got) = %v", tt.name, d) + } } }) } @@ -278,7 +292,7 @@ func TestHasResultReference(t *testing.T) { if !ok { t.Fatalf("expected to find expressions but didn't find any") } - got, _ := v1beta1.NewResultRefs(expressions) + got := v1beta1.NewResultRefs(expressions) sort.Slice(got, func(i, j int) bool { if got[i].PipelineTask > got[j].PipelineTask { return false diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 83f694802fe..8c417dde677 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -1773,3 +1773,77 @@ func TestReconcileWithTaskResults(t *testing.T) { t.Errorf("expected to see TaskRun %v created. Diff %s", expectedTaskRunName, d) } } + +func TestReconcileWithTaskResultsEmbeddedNoneStarted(t *testing.T) { + names.TestingSeed() + prs := []*v1alpha1.PipelineRun{tb.PipelineRun("test-pipeline-run-different-service-accs", "foo", + tb.PipelineRunSpec("test-pipeline", + tb.PipelineRunPipelineSpec( + tb.PipelineParamSpec("foo", v1alpha1.ParamTypeString), + tb.PipelineTask("a-task", "a-task"), + tb.PipelineTask("b-task", "b-task", + tb.PipelineTaskParam("bParam", "$(params.foo)/baz@$(tasks.a-task.results.A_RESULT)"), + ), + ), + tb.PipelineRunParam("foo", "bar"), + tb.PipelineRunServiceAccountName("test-sa-0"), + ), + )} + ts := []*v1alpha1.Task{ + tb.Task("a-task", "foo"), + tb.Task("b-task", "foo", + tb.TaskSpec( + tb.TaskInputs(tb.InputsParamSpec("bParam", v1alpha1.ParamTypeString)), + ), + ), + } + d := test.Data{ + PipelineRuns: prs, + Tasks: ts, + } + testAssets, cancel := getPipelineRunController(t, d) + defer cancel() + c := testAssets.Controller + clients := testAssets.Clients + err := c.Reconciler.Reconcile(context.Background(), "foo/test-pipeline-run-different-service-accs") + if err != nil { + t.Errorf("Did not expect to see error when reconciling completed PipelineRun but saw %s", err) + } + // Check that the PipelineRun was reconciled correctly + reconciledRun, err := clients.Pipeline.TektonV1alpha1().PipelineRuns("foo").Get("test-pipeline-run-different-service-accs", metav1.GetOptions{}) + if err != nil { + t.Fatalf("Somehow had error getting completed reconciled run out of fake client: %s", err) + } + + if !reconciledRun.Status.GetCondition(apis.ConditionSucceeded).IsUnknown() { + t.Errorf("Expected PipelineRun to be running, but condition status is %s", reconciledRun.Status.GetCondition(apis.ConditionSucceeded)) + } + + // Since b-task is dependent on a-task, via the results, only a-task should run + expectedTaskRunName := "test-pipeline-run-different-service-accs-a-task-9l9zj" + expectedTaskRun := tb.TaskRun(expectedTaskRunName, "foo", + tb.TaskRunOwnerReference("PipelineRun", "test-pipeline-run-different-service-accs", + tb.OwnerReferenceAPIVersion("tekton.dev/v1alpha1"), + tb.Controller, tb.BlockOwnerDeletion, + ), + tb.TaskRunLabel("tekton.dev/pipeline", "test-pipeline-run-different-service-accs"), + tb.TaskRunLabel("tekton.dev/pipelineRun", "test-pipeline-run-different-service-accs"), + tb.TaskRunLabel("tekton.dev/pipelineTask", "a-task"), + tb.TaskRunSpec( + tb.TaskRunTaskRef("a-task", tb.TaskRefKind(v1beta1.NamespacedTaskKind)), + tb.TaskRunServiceAccountName("test-sa-0"), + ), + ) + // Check that the expected TaskRun was created (only) + actual, err := clients.Pipeline.TektonV1alpha1().TaskRuns("foo").List(metav1.ListOptions{}) + if err != nil { + t.Fatalf("Failure to list TaskRun's %s", err) + } + if len(actual.Items) != 1 { + t.Fatalf("Expected 1 TaskRuns got %d", len(actual.Items)) + } + actualTaskRun := actual.Items[0] + if d := cmp.Diff(expectedTaskRun, &actualTaskRun); d != "" { + t.Errorf("expected to see TaskRun %v created. Diff (-want, +got): %s", expectedTaskRun, d) + } +} diff --git a/pkg/reconciler/pipelinerun/resources/resultrefresolution.go b/pkg/reconciler/pipelinerun/resources/resultrefresolution.go index 79956ae85ee..28114acfa58 100644 --- a/pkg/reconciler/pipelinerun/resources/resultrefresolution.go +++ b/pkg/reconciler/pipelinerun/resources/resultrefresolution.go @@ -59,18 +59,16 @@ func extractResultRefsForParam(pipelineRunState PipelineRunState, param v1beta1. } func extractResultRefs(expressions []string, pipelineRunState PipelineRunState) (ResolvedResultRefs, error) { - if resultRefs, err := v1beta1.NewResultRefs(expressions); err == nil { - var resolvedResultRefs ResolvedResultRefs - for _, resultRef := range resultRefs { - resolvedResultRef, err := resolveResultRef(pipelineRunState, resultRef) - if err != nil { - return nil, err - } - resolvedResultRefs = append(resolvedResultRefs, resolvedResultRef) + resultRefs := v1beta1.NewResultRefs(expressions) + var resolvedResultRefs ResolvedResultRefs + for _, resultRef := range resultRefs { + resolvedResultRef, err := resolveResultRef(pipelineRunState, resultRef) + if err != nil { + return nil, err } - return removeDup(resolvedResultRefs), nil + resolvedResultRefs = append(resolvedResultRefs, resolvedResultRef) } - return nil, nil + return removeDup(resolvedResultRefs), nil } func removeDup(refs ResolvedResultRefs) ResolvedResultRefs {