Determine correct graph when params and results are mixed 🥄

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.

examples/v1beta1/pipelineruns/pipelinerun-results-with-params.yaml

@@ -21,16 +21,14 @@ spec:
           script: |
             echo -n "suffix" > $(results.suffix.path)
     - name: do-something
-      runAfter:
-      - generate-suffix
       taskSpec:
         params:
         - name: arg
         steps:
         - 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)"

pkg/apis/pipeline/v1alpha1/pipeline_types.go

@@ -170,10 +170,9 @@ func (pt PipelineTask) Deps() []string {
 		for _, param := range cond.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)
 				}
 			}
 		}
@@ -182,10 +181,9 @@ func (pt PipelineTask) Deps() []string {
 	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)
 			}
 		}
 	}

pkg/apis/pipeline/v1beta1/pipeline_types.go

@@ -155,10 +155,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)
 				}
 			}
 		}
@@ -167,10 +166,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)
 			}
 		}
 	}

pkg/apis/pipeline/v1beta1/pipeline_validation.go

@@ -331,8 +331,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)
 					}
 				}
 			}
@@ -358,8 +359,9 @@ func validatePipelineResults(results []PipelineResult) 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)
 				}
 			}
 		}

pkg/apis/pipeline/v1beta1/resultref.go

@@ -41,20 +41,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

pkg/apis/pipeline/v1beta1/resultref_test.go

@@ -29,10 +29,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",
@@ -51,9 +50,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",
@@ -69,9 +67,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",
@@ -90,9 +87,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",
@@ -102,10 +118,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",
@@ -115,10 +130,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",
@@ -128,10 +142,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",
@@ -141,23 +154,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)
+				}
 			}
 		})
 	}
@@ -281,7 +295,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

pkg/reconciler/pipelinerun/pipelinerun_test.go

@@ -1891,6 +1891,85 @@ func TestReconcileWithTaskResults(t *testing.T) {
 	}
 }
 
+func TestReconcileWithTaskResultsEmbeddedNoneStarted(t *testing.T) {
+	names.TestingSeed()
+	prs := []*v1alpha1.PipelineRun{tb.PipelineRun("test-pipeline-run-different-service-accs", tb.PipelineRunNamespace("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", tb.TaskNamespace("foo"),
+			tb.TaskSpec(
+				tb.TaskResults("A_RESULT", ""),
+			),
+		),
+		tb.Task("b-task", tb.TaskNamespace("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,
+		tb.TaskRunNamespace("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)
+	}
+}
+
 func TestReconcileWithPipelineResults(t *testing.T) {
 	names.TestingSeed()
 	ps := []*v1alpha1.Pipeline{tb.Pipeline("test-pipeline", tb.PipelineNamespace("foo"), tb.PipelineSpec(

pkg/reconciler/pipelinerun/resources/resultrefresolution.go

@@ -76,18 +76,16 @@ func extractResultRefsForPipelineResult(pipelineRunState PipelineRunState, resul
 }
 
 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 {