Fix 3 bugs with results

examples/v1beta1/pipelineruns/pipelinerun-results-with-params.yml → 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

@@ -152,6 +152,25 @@ func (pt PipelineTask) Deps() []string {
 		for _, rd := range cond.Resources {
 			deps = append(deps, rd.From...)
 		}
+		for _, param := range cond.Params {
+			expressions, ok := GetVarSubstitutionExpressionsForParam(param)
+			if ok {
+				resultRefs := NewResultRefs(expressions)
+				for _, resultRef := range resultRefs {
+					deps = append(deps, resultRef.PipelineTask)
+				}
+			}
+		}
+	}
+	// Add any dependents from task results
+	for _, param := range pt.Params {
+		expressions, ok := GetVarSubstitutionExpressionsForParam(param)
+		if ok {
+			resultRefs := NewResultRefs(expressions)
+			for _, resultRef := range resultRefs {
+				deps = append(deps, resultRef.PipelineTask)
+			}
+		}
 	}
 	return deps
 }

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

@@ -33,30 +33,34 @@ const (
 	// ResultTaskPart Constant used to define the "tasks" part of a pipeline result reference
 	ResultTaskPart = "tasks"
 	// ResultResultPart Constant used to define the "results" part of a pipeline result reference
-	ResultResultPart           = "results"
-	variableSubstitutionFormat = `\$\([A-Za-z0-9-]+(\.[A-Za-z0-9-]+)*\)`
+	ResultResultPart = "results"
+	// TODO(#2462) use one regex across all substitutions
+	variableSubstitutionFormat = `\$\([_a-zA-Z0-9.-]+(\.[_a-zA-Z0-9.-]+)*\)`
 )
 
 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
+// LooksLikeContainsResultRefs attempts to check if param or a pipeline result looks like it contains any
 // result references.
 // This is useful if we want to make sure the param looks like a ResultReference before
 // performing strict validation

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,19 +154,21 @@ 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 {
-				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 !ok && tt.want != nil {
+				t.Fatalf("expected to find expressions but didn't find any")
+			} else {
+				got := v1beta1.NewResultRefs(expressions)
 				if d := cmp.Diff(tt.want, got); d != "" {
 					t.Errorf("TestNewResultReference/%s (-want, +got) = %v", tt.name, d)
 				}
@@ -205,6 +220,23 @@ func TestHasResultReference(t *testing.T) {
 					Result:       "sum-result",
 				},
 			},
+		}, {
+			name: "Test valid expression with underscores",
+			args: args{
+				param: v1beta1.Param{
+					Name: "param",
+					Value: v1beta1.ArrayOrString{
+						Type:      v1beta1.ParamTypeString,
+						StringVal: "$(tasks.sum-task.results.sum_result)",
+					},
+				},
+			},
+			wantRef: []*v1beta1.ResultRef{
+				{
+					PipelineTask: "sum-task",
+					Result:       "sum_result",
+				},
+			},
 		}, {
 			name: "Test invalid expression: param substitution shouldn't be considered result ref",
 			args: args{
@@ -260,20 +292,21 @@ func TestHasResultReference(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			expressions, ok := v1beta1.GetVarSubstitutionExpressionsForParam(tt.args.param)
-			if ok {
-				got, _ := v1beta1.NewResultRefs(expressions)
-				sort.Slice(got, func(i, j int) bool {
-					if got[i].PipelineTask > got[j].PipelineTask {
-						return false
-					}
-					if got[i].Result > got[j].Result {
-						return false
-					}
-					return true
-				})
-				if d := cmp.Diff(tt.wantRef, got); d != "" {
-					t.Errorf("TestHasResultReference/%s (-want, +got) = %v", tt.name, d)
+			if !ok {
+				t.Fatalf("expected to find expressions but didn't find any")
+			}
+			got := v1beta1.NewResultRefs(expressions)
+			sort.Slice(got, func(i, j int) bool {
+				if got[i].PipelineTask > got[j].PipelineTask {
+					return false
+				}
+				if got[i].Result > got[j].Result {
+					return false
 				}
+				return true
+			})
+			if d := cmp.Diff(tt.wantRef, got); d != "" {
+				t.Errorf("TestHasResultReference/%s (-want, +got) = %v", tt.name, d)
 			}
 		})
 	}

pkg/reconciler/pipeline/dag/dag_test.go

@@ -42,8 +42,15 @@ func testGraph(t *testing.T) *dag.Graph {
 	}, {
 		Name: "b",
 	}, {
-		Name:     "w",
-		RunAfter: []string{"b", "y"},
+		Name: "w",
+		Params: []v1alpha1.Param{{
+			Name: "foo",
+			Value: v1alpha1.ArrayOrString{
+				Type:      v1alpha1.ParamTypeString,
+				StringVal: "$(tasks.y.results.bar)",
+			},
+		}},
+		RunAfter: []string{"b"},
 	}, {
 		Name:     "x",
 		RunAfter: []string{"a"},