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 {