diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_types.go b/pkg/apis/pipeline/v1alpha1/pipeline_types.go index 4c8379c2b6d..2ef98a06dd1 100644 --- a/pkg/apis/pipeline/v1alpha1/pipeline_types.go +++ b/pkg/apis/pipeline/v1alpha1/pipeline_types.go @@ -164,9 +164,12 @@ func (pt PipelineTask) Deps() []string { } // Add any dependents from task results for _, param := range pt.Params { - if resultRefs, err := v1beta1.NewResultRefs(param); err == nil { - for _, resultRef := range resultRefs { - deps = append(deps, resultRef.PipelineTask) + expressions, ok := v1beta1.GetVarSubstitutionExpressionsForParam(param) + if ok { + if resultRefs, err := v1beta1.NewResultRefs(expressions); err == nil { + for _, resultRef := range resultRefs { + deps = append(deps, resultRef.PipelineTask) + } } } } diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_validation.go b/pkg/apis/pipeline/v1alpha1/pipeline_validation.go index f0d099a43ab..a0a0e45ad36 100644 --- a/pkg/apis/pipeline/v1alpha1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1alpha1/pipeline_validation.go @@ -140,9 +140,12 @@ func validateGraph(tasks []PipelineTask) error { func validateParamResults(tasks []PipelineTask) error { for _, task := range tasks { for _, param := range task.Params { - if v1beta1.LooksLikeContainsResultRefs(param) { - if _, err := v1beta1.NewResultRefs(param); err != nil { - return err + expressions, ok := v1beta1.GetVarSubstitutionExpressionsForParam(param) + if ok { + if v1beta1.LooksLikeContainsResultRefs(expressions) { + if _, err := v1beta1.NewResultRefs(expressions); err != nil { + return err + } } } } diff --git a/pkg/apis/pipeline/v1beta1/param_types.go b/pkg/apis/pipeline/v1beta1/param_types.go index 50312647a85..f2c63a664c5 100644 --- a/pkg/apis/pipeline/v1beta1/param_types.go +++ b/pkg/apis/pipeline/v1beta1/param_types.go @@ -20,8 +20,6 @@ import ( "context" "encoding/json" "fmt" - "regexp" - "strings" resource "github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1" ) @@ -142,108 +140,3 @@ func NewArrayOrString(value string, values ...string) ArrayOrString { StringVal: value, } } - -// ResultRef is a type that represents a reference to a task run result -type ResultRef struct { - PipelineTask string - Result string -} - -const ( - resultExpressionFormat = "tasks..results." - // 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-]+)*\)` -) - -var variableSubstitutionRegex = regexp.MustCompile(variableSubstitutionFormat) - -// NewResultRefs extracts all ResultReferences from param. -// If the ResultReference can be extracted, they are returned. Otherwise an error is returned -func NewResultRefs(param Param) ([]*ResultRef, error) { - substitutionExpressions, ok := getVarSubstitutionExpressions(param) - if !ok { - return nil, fmt.Errorf("Invalid result reference expression: must contain variable substitution %q", resultExpressionFormat) - } - var resultRefs []*ResultRef - for _, expression := range substitutionExpressions { - pipelineTask, result, err := parseExpression(expression) - if err != nil { - return nil, fmt.Errorf("Invalid result reference expression: %v", err) - } - resultRefs = append(resultRefs, &ResultRef{ - PipelineTask: pipelineTask, - Result: result, - }) - } - return resultRefs, nil -} - -// LooksLikeContainsResultRefs attempts to check if param 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 -func LooksLikeContainsResultRefs(param Param) bool { - extractedExpressions, ok := getVarSubstitutionExpressions(param) - if !ok { - return false - } - for _, expression := range extractedExpressions { - if looksLikeResultRef(expression) { - return true - } - } - return false -} - -func looksLikeResultRef(expression string) bool { - return strings.HasPrefix(expression, "task") && strings.Contains(expression, ".result") -} - -// getVarSubstitutionExpressions extracts all the value between "$(" and ")"" -func getVarSubstitutionExpressions(param Param) ([]string, bool) { - var allExpressions []string - switch param.Value.Type { - case ParamTypeArray: - // array type - for _, value := range param.Value.ArrayVal { - expressions := variableSubstitutionRegex.FindAllString(value, -1) - if expressions == nil { - continue - } - for _, expression := range expressions { - allExpressions = append(allExpressions, stripVarSubExpression(expression)) - } - } - if len(allExpressions) == 0 { - return nil, false - } - return allExpressions, true - case ParamTypeString: - // string type - expressions := variableSubstitutionRegex.FindAllString(param.Value.StringVal, -1) - if expressions == nil { - return nil, false - } - for _, expression := range expressions { - allExpressions = append(allExpressions, stripVarSubExpression(expression)) - } - return allExpressions, true - default: - return nil, false - } -} - -func stripVarSubExpression(expression string) string { - return strings.TrimSuffix(strings.TrimPrefix(expression, "$("), ")") -} - -func parseExpression(substitutionExpression string) (string, string, error) { - subExpressions := strings.Split(substitutionExpression, ".") - if len(subExpressions) != 4 || subExpressions[0] != ResultTaskPart || subExpressions[2] != ResultResultPart { - return "", "", fmt.Errorf("Must be of the form %q", resultExpressionFormat) - } - return subExpressions[1], subExpressions[3], nil -} diff --git a/pkg/apis/pipeline/v1beta1/param_types_test.go b/pkg/apis/pipeline/v1beta1/param_types_test.go index f6cc394e030..021fc0a06ed 100644 --- a/pkg/apis/pipeline/v1beta1/param_types_test.go +++ b/pkg/apis/pipeline/v1beta1/param_types_test.go @@ -20,7 +20,6 @@ import ( "context" "encoding/json" "reflect" - "sort" "testing" "github.com/google/go-cmp/cmp" @@ -188,368 +187,3 @@ func TestArrayOrString_MarshalJSON(t *testing.T) { } } } - -func TestNewResultReference(t *testing.T) { - type args struct { - param v1beta1.Param - } - tests := []struct { - name string - args args - want []*v1beta1.ResultRef - wantErr bool - }{ - { - name: "Test valid expression", - args: args{ - param: v1beta1.Param{ - Name: "param", - Value: v1beta1.ArrayOrString{ - Type: v1beta1.ParamTypeString, - StringVal: "$(tasks.sumTask.results.sumResult)", - }, - }, - }, - want: []*v1beta1.ResultRef{ - { - PipelineTask: "sumTask", - Result: "sumResult", - }, - }, - wantErr: false, - }, { - name: "Test valid expression: substitution within string", - args: args{ - param: v1beta1.Param{ - Name: "param", - Value: v1beta1.ArrayOrString{ - Type: v1beta1.ParamTypeString, - StringVal: "sum-will-go-here -> $(tasks.sumTask.results.sumResult)", - }, - }, - }, - want: []*v1beta1.ResultRef{ - { - PipelineTask: "sumTask", - Result: "sumResult", - }, - }, - wantErr: false, - }, { - name: "Test valid expression: multiple substitution", - args: args{ - param: v1beta1.Param{ - Name: "param", - Value: v1beta1.ArrayOrString{ - Type: v1beta1.ParamTypeString, - StringVal: "$(tasks.sumTask1.results.sumResult) and another $(tasks.sumTask2.results.sumResult)", - }, - }, - }, - want: []*v1beta1.ResultRef{ - { - PipelineTask: "sumTask1", - Result: "sumResult", - }, { - PipelineTask: "sumTask2", - Result: "sumResult", - }, - }, - wantErr: false, - }, { - name: "Test invalid expression: first separator typo", - args: args{ - param: v1beta1.Param{ - Name: "param", - Value: v1beta1.ArrayOrString{ - Type: v1beta1.ParamTypeString, - StringVal: "$(task.sumTasks.results.sumResult)", - }, - }, - }, - want: nil, - wantErr: true, - }, { - name: "Test invalid expression: third separator typo", - args: args{ - param: v1beta1.Param{ - Name: "param", - Value: v1beta1.ArrayOrString{ - Type: v1beta1.ParamTypeString, - StringVal: "$(tasks.sumTasks.result.sumResult)", - }, - }, - }, - want: nil, - wantErr: true, - }, { - name: "Test invalid expression: param substitution shouldn't be considered result ref", - args: args{ - param: v1beta1.Param{ - Name: "param", - Value: v1beta1.ArrayOrString{ - Type: v1beta1.ParamTypeString, - StringVal: "$(params.paramName)", - }, - }, - }, - want: nil, - wantErr: true, - }, { - name: "Test invalid expression: One bad and good result substitution", - args: args{ - param: v1beta1.Param{ - Name: "param", - Value: v1beta1.ArrayOrString{ - Type: v1beta1.ParamTypeString, - StringVal: "good -> $(tasks.sumTask1.results.sumResult) bad-> $(task.sumTask2.results.sumResult)", - }, - }, - }, - want: nil, - wantErr: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := v1beta1.NewResultRefs(tt.args.param) - 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) - } - }) - } -} - -func TestHasResultReference(t *testing.T) { - type args struct { - param v1beta1.Param - } - tests := []struct { - name string - args args - wantRef []*v1beta1.ResultRef - }{ - { - name: "Test valid expression", - args: args{ - param: v1beta1.Param{ - Name: "param", - Value: v1beta1.ArrayOrString{ - Type: v1beta1.ParamTypeString, - StringVal: "$(tasks.sumTask.results.sumResult)", - }, - }, - }, - wantRef: []*v1beta1.ResultRef{ - { - PipelineTask: "sumTask", - Result: "sumResult", - }, - }, - }, { - name: "Test valid expression with dashes", - 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{ - param: v1beta1.Param{ - Name: "param", - Value: v1beta1.ArrayOrString{ - Type: v1beta1.ParamTypeString, - StringVal: "$(params.paramName)", - }, - }, - }, - wantRef: nil, - }, { - name: "Test valid expression in array", - args: args{ - param: v1beta1.Param{ - Name: "param", - Value: v1beta1.ArrayOrString{ - Type: v1beta1.ParamTypeArray, - ArrayVal: []string{"$(tasks.sumTask.results.sumResult)", "$(tasks.sumTask2.results.sumResult2)"}, - }, - }, - }, - wantRef: []*v1beta1.ResultRef{ - { - PipelineTask: "sumTask", - Result: "sumResult", - }, - { - PipelineTask: "sumTask2", - Result: "sumResult2", - }, - }, - }, { - name: "Test valid expression in array - no ref in first element", - args: args{ - param: v1beta1.Param{ - Name: "param", - Value: v1beta1.ArrayOrString{ - Type: v1beta1.ParamTypeArray, - ArrayVal: []string{"1", "$(tasks.sumTask2.results.sumResult2)"}, - }, - }, - }, - wantRef: []*v1beta1.ResultRef{ - { - PipelineTask: "sumTask2", - Result: "sumResult2", - }, - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, _ := v1beta1.NewResultRefs(tt.args.param) - 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) - } - }) - } -} - -func TestLooksLikeResultRef(t *testing.T) { - type args struct { - param v1beta1.Param - } - tests := []struct { - name string - args args - want bool - }{ - { - name: "test expression that is a result ref", - args: args{ - param: v1beta1.Param{ - Name: "param", - Value: v1beta1.ArrayOrString{ - Type: v1beta1.ParamTypeString, - StringVal: "$(tasks.sumTasks.results.sumResult)", - }, - }, - }, - want: true, - }, { - name: "test expression: looks like result ref, but typo in 'task' separator", - args: args{ - param: v1beta1.Param{ - Name: "param", - Value: v1beta1.ArrayOrString{ - Type: v1beta1.ParamTypeString, - StringVal: "$(task.sumTasks.results.sumResult)", - }, - }, - }, - want: true, - }, { - name: "test expression: looks like result ref, but typo in 'results' separator", - args: args{ - param: v1beta1.Param{ - Name: "param", - Value: v1beta1.ArrayOrString{ - Type: v1beta1.ParamTypeString, - StringVal: "$(tasks.sumTasks.result.sumResult)", - }, - }, - }, - want: true, - }, { - name: "test expression: missing 'task' separator", - args: args{ - param: v1beta1.Param{ - Name: "param", - Value: v1beta1.ArrayOrString{ - Type: v1beta1.ParamTypeString, - StringVal: "$(sumTasks.results.sumResult)", - }, - }, - }, - want: false, - }, { - name: "test expression: missing variable substitution", - args: args{ - param: v1beta1.Param{ - Name: "param", - Value: v1beta1.ArrayOrString{ - Type: v1beta1.ParamTypeString, - StringVal: "tasks.sumTasks.results.sumResult", - }, - }, - }, - want: false, - }, { - name: "test expression: param substitution shouldn't be considered result ref", - args: args{ - param: v1beta1.Param{ - Name: "param", - Value: v1beta1.ArrayOrString{ - Type: v1beta1.ParamTypeString, - StringVal: "$(params.someParam)", - }, - }, - }, - want: false, - }, { - name: "test expression: one good ref, one bad one should return true", - args: args{ - param: v1beta1.Param{ - Name: "param", - Value: v1beta1.ArrayOrString{ - Type: v1beta1.ParamTypeString, - StringVal: "$(tasks.sumTasks.results.sumResult) $(task.sumTasks.results.sumResult)", - }, - }, - }, - want: true, - }, { - name: "test expression: inside array parameter", - args: args{ - param: v1beta1.Param{ - Name: "param", - Value: v1beta1.ArrayOrString{ - Type: v1beta1.ParamTypeArray, - ArrayVal: []string{"$(tasks.sumTask.results.sumResult)", "$(tasks.sumTask2.results.sumResult2)"}, - }, - }, - }, - want: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := v1beta1.LooksLikeContainsResultRefs(tt.args.param); got != tt.want { - t.Errorf("LooksLikeContainsResultRefs() = %v, want %v", got, tt.want) - } - }) - } -} diff --git a/pkg/apis/pipeline/v1beta1/resultref.go b/pkg/apis/pipeline/v1beta1/resultref.go new file mode 100644 index 00000000000..3a189681560 --- /dev/null +++ b/pkg/apis/pipeline/v1beta1/resultref.go @@ -0,0 +1,113 @@ +/* +Copyright 2019 The Tekton Authors +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1beta1 + +import ( + "fmt" + "regexp" + "strings" +) + +// ResultRef is a type that represents a reference to a task run result +type ResultRef struct { + PipelineTask string + Result string +} + +const ( + resultExpressionFormat = "tasks..results." + // 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-]+)*\)` +) + +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) { + 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) + } + resultRefs = append(resultRefs, &ResultRef{ + PipelineTask: pipelineTask, + Result: result, + }) + } + return resultRefs, nil +} + +// 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 +func LooksLikeContainsResultRefs(expressions []string) bool { + for _, expression := range expressions { + if looksLikeResultRef(expression) { + return true + } + } + return false +} + +func looksLikeResultRef(expression string) bool { + return strings.HasPrefix(expression, "task") && strings.Contains(expression, ".result") +} + +// GetVarSubstitutionExpressionsForParam extracts all the value between "$(" and ")"" for a parameter +func GetVarSubstitutionExpressionsForParam(param Param) ([]string, bool) { + var allExpressions []string + switch param.Value.Type { + case ParamTypeArray: + // array type + for _, value := range param.Value.ArrayVal { + allExpressions = append(allExpressions, validateString(value)...) + } + case ParamTypeString: + // string type + allExpressions = append(allExpressions, validateString(param.Value.StringVal)...) + default: + return nil, false + } + return allExpressions, len(allExpressions) != 0 +} + +func validateString(value string) []string { + expressions := variableSubstitutionRegex.FindAllString(value, -1) + if expressions == nil { + return nil + } + var result []string + for _, expression := range expressions { + result = append(result, stripVarSubExpression(expression)) + } + return result +} + +func stripVarSubExpression(expression string) string { + return strings.TrimSuffix(strings.TrimPrefix(expression, "$("), ")") +} + +func parseExpression(substitutionExpression string) (string, string, error) { + subExpressions := strings.Split(substitutionExpression, ".") + if len(subExpressions) != 4 || subExpressions[0] != ResultTaskPart || subExpressions[2] != ResultResultPart { + return "", "", fmt.Errorf("Must be of the form %q", resultExpressionFormat) + } + return subExpressions[1], subExpressions[3], nil +} diff --git a/pkg/apis/pipeline/v1beta1/resultref_test.go b/pkg/apis/pipeline/v1beta1/resultref_test.go new file mode 100644 index 00000000000..c6febcaa813 --- /dev/null +++ b/pkg/apis/pipeline/v1beta1/resultref_test.go @@ -0,0 +1,398 @@ +/* +Copyright 2019 The Tekton Authors +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1beta1_test + +import ( + "sort" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" +) + +func TestNewResultReference(t *testing.T) { + type args struct { + param v1beta1.Param + } + tests := []struct { + name string + args args + want []*v1beta1.ResultRef + wantErr bool + }{ + { + name: "Test valid expression", + args: args{ + param: v1beta1.Param{ + Name: "param", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeString, + StringVal: "$(tasks.sumTask.results.sumResult)", + }, + }, + }, + want: []*v1beta1.ResultRef{ + { + PipelineTask: "sumTask", + Result: "sumResult", + }, + }, + wantErr: false, + }, { + name: "Test valid expression: substitution within string", + args: args{ + param: v1beta1.Param{ + Name: "param", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeString, + StringVal: "sum-will-go-here -> $(tasks.sumTask.results.sumResult)", + }, + }, + }, + want: []*v1beta1.ResultRef{ + { + PipelineTask: "sumTask", + Result: "sumResult", + }, + }, + wantErr: false, + }, { + name: "Test valid expression: multiple substitution", + args: args{ + param: v1beta1.Param{ + Name: "param", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeString, + StringVal: "$(tasks.sumTask1.results.sumResult) and another $(tasks.sumTask2.results.sumResult)", + }, + }, + }, + want: []*v1beta1.ResultRef{ + { + PipelineTask: "sumTask1", + Result: "sumResult", + }, { + PipelineTask: "sumTask2", + Result: "sumResult", + }, + }, + wantErr: false, + }, { + name: "Test invalid expression: first separator typo", + args: args{ + param: v1beta1.Param{ + Name: "param", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeString, + StringVal: "$(task.sumTasks.results.sumResult)", + }, + }, + }, + want: nil, + wantErr: true, + }, { + name: "Test invalid expression: third separator typo", + args: args{ + param: v1beta1.Param{ + Name: "param", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeString, + StringVal: "$(tasks.sumTasks.result.sumResult)", + }, + }, + }, + want: nil, + wantErr: true, + }, { + name: "Test invalid expression: param substitution shouldn't be considered result ref", + args: args{ + param: v1beta1.Param{ + Name: "param", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeString, + StringVal: "$(params.paramName)", + }, + }, + }, + want: nil, + wantErr: true, + }, { + name: "Test invalid expression: One bad and good result substitution", + args: args{ + param: v1beta1.Param{ + Name: "param", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeString, + StringVal: "good -> $(tasks.sumTask1.results.sumResult) bad-> $(task.sumTask2.results.sumResult)", + }, + }, + }, + want: nil, + wantErr: true, + }, + } + 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 d := cmp.Diff(tt.want, got); d != "" { + t.Errorf("TestNewResultReference/%s (-want, +got) = %v", tt.name, d) + } + } + }) + } +} + +func TestHasResultReference(t *testing.T) { + type args struct { + param v1beta1.Param + } + tests := []struct { + name string + args args + wantRef []*v1beta1.ResultRef + }{ + { + name: "Test valid expression", + args: args{ + param: v1beta1.Param{ + Name: "param", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeString, + StringVal: "$(tasks.sumTask.results.sumResult)", + }, + }, + }, + wantRef: []*v1beta1.ResultRef{ + { + PipelineTask: "sumTask", + Result: "sumResult", + }, + }, + }, { + name: "Test valid expression with dashes", + 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{ + param: v1beta1.Param{ + Name: "param", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeString, + StringVal: "$(params.paramName)", + }, + }, + }, + wantRef: nil, + }, { + name: "Test valid expression in array", + args: args{ + param: v1beta1.Param{ + Name: "param", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeArray, + ArrayVal: []string{"$(tasks.sumTask.results.sumResult)", "$(tasks.sumTask2.results.sumResult2)"}, + }, + }, + }, + wantRef: []*v1beta1.ResultRef{ + { + PipelineTask: "sumTask", + Result: "sumResult", + }, + { + PipelineTask: "sumTask2", + Result: "sumResult2", + }, + }, + }, { + name: "Test valid expression in array - no ref in first element", + args: args{ + param: v1beta1.Param{ + Name: "param", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeArray, + ArrayVal: []string{"1", "$(tasks.sumTask2.results.sumResult2)"}, + }, + }, + }, + wantRef: []*v1beta1.ResultRef{ + { + PipelineTask: "sumTask2", + Result: "sumResult2", + }, + }, + }, + } + 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) + } + } + }) + } +} + +func TestLooksLikeResultRef(t *testing.T) { + type args struct { + param v1beta1.Param + } + tests := []struct { + name string + args args + want bool + }{ + { + name: "test expression that is a result ref", + args: args{ + param: v1beta1.Param{ + Name: "param", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeString, + StringVal: "$(tasks.sumTasks.results.sumResult)", + }, + }, + }, + want: true, + }, { + name: "test expression: looks like result ref, but typo in 'task' separator", + args: args{ + param: v1beta1.Param{ + Name: "param", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeString, + StringVal: "$(task.sumTasks.results.sumResult)", + }, + }, + }, + want: true, + }, { + name: "test expression: looks like result ref, but typo in 'results' separator", + args: args{ + param: v1beta1.Param{ + Name: "param", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeString, + StringVal: "$(tasks.sumTasks.result.sumResult)", + }, + }, + }, + want: true, + }, { + name: "test expression: missing 'task' separator", + args: args{ + param: v1beta1.Param{ + Name: "param", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeString, + StringVal: "$(sumTasks.results.sumResult)", + }, + }, + }, + want: false, + }, { + name: "test expression: missing variable substitution", + args: args{ + param: v1beta1.Param{ + Name: "param", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeString, + StringVal: "tasks.sumTasks.results.sumResult", + }, + }, + }, + want: false, + }, { + name: "test expression: param substitution shouldn't be considered result ref", + args: args{ + param: v1beta1.Param{ + Name: "param", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeString, + StringVal: "$(params.someParam)", + }, + }, + }, + want: false, + }, { + name: "test expression: one good ref, one bad one should return true", + args: args{ + param: v1beta1.Param{ + Name: "param", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeString, + StringVal: "$(tasks.sumTasks.results.sumResult) $(task.sumTasks.results.sumResult)", + }, + }, + }, + want: true, + }, { + name: "test expression: inside array parameter", + args: args{ + param: v1beta1.Param{ + Name: "param", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeArray, + ArrayVal: []string{"$(tasks.sumTask.results.sumResult)", "$(tasks.sumTask2.results.sumResult2)"}, + }, + }, + }, + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + expressions, ok := v1beta1.GetVarSubstitutionExpressionsForParam(tt.args.param) + if ok { + if got := v1beta1.LooksLikeContainsResultRefs(expressions); got != tt.want { + t.Errorf("LooksLikeContainsResultRefs() = %v, want %v", got, tt.want) + } + } else if tt.want { + t.Errorf("LooksLikeContainsResultRefs() = %v, want %v", false, tt.want) + } + }) + } +} diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index e52481e7ec4..0f4ae747ae4 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -157,6 +157,10 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { var merr error if pr.IsDone() { + // We may be reading a version of the object that was stored at an older version + // and may not have had all of the assumed default specified. + pr.SetDefaults(contexts.WithUpgradeViaDefaulting(ctx)) + if err := artifacts.CleanupArtifactStorage(pr, c.KubeClientSet, c.Logger); err != nil { c.Logger.Errorf("Failed to delete PVC for PipelineRun %s: %v", pr.Name, err) return err diff --git a/pkg/reconciler/pipelinerun/resources/resultrefresolution.go b/pkg/reconciler/pipelinerun/resources/resultrefresolution.go index e7054b4a203..79956ae85ee 100644 --- a/pkg/reconciler/pipelinerun/resources/resultrefresolution.go +++ b/pkg/reconciler/pipelinerun/resources/resultrefresolution.go @@ -48,10 +48,18 @@ func ResolveResultRefs(pipelineRunState PipelineRunState, targets PipelineRunSta return removeDup(allResolvedResultRefs), nil } -// extractResultRefsFromParam resolves any ResultReference that are found in param +// extractResultRefsForParam resolves any ResultReference that are found in param // Returns nil if none are found -func extractResultRefsFromParam(pipelineRunState PipelineRunState, param v1beta1.Param) (ResolvedResultRefs, error) { - if resultRefs, err := v1beta1.NewResultRefs(param); err == nil { +func extractResultRefsForParam(pipelineRunState PipelineRunState, param v1beta1.Param) (ResolvedResultRefs, error) { + expressions, ok := v1beta1.GetVarSubstitutionExpressionsForParam(param) + if ok { + return extractResultRefs(expressions, pipelineRunState) + } + return nil, nil +} + +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) @@ -85,7 +93,7 @@ func removeDup(refs ResolvedResultRefs) ResolvedResultRefs { func convertParamsToResultRefs(pipelineRunState PipelineRunState, target *ResolvedPipelineRunTask) (ResolvedResultRefs, error) { var resolvedParams ResolvedResultRefs for _, param := range target.PipelineTask.Params { - resolvedResultRefs, err := extractResultRefsFromParam(pipelineRunState, param) + resolvedResultRefs, err := extractResultRefsForParam(pipelineRunState, param) if err != nil { return nil, fmt.Errorf("unable to find result referenced by param %q in pipeline task %q: %w", param.Name, target.PipelineTask.Name, err) } diff --git a/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go b/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go index 294de677865..f25a29bd99a 100644 --- a/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go +++ b/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go @@ -1,7 +1,24 @@ +/* +Copyright 2019 The Tekton Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + package resources import ( "fmt" + "sort" "strings" "testing" @@ -256,7 +273,11 @@ func TestTaskParamResolver_ResolveResultRefs(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Logf("test name: %s\n", tt.name) - got, err := extractResultRefsFromParam(tt.fields.pipelineRunState, tt.args.param) + got, err := extractResultRefsForParam(tt.fields.pipelineRunState, tt.args.param) + // sort result ref based on task name to garantee an certain order + sort.SliceStable(got, func(i, j int) bool { + return strings.Compare(got[i].FromTaskRun, got[j].FromTaskRun) < 0 + }) if (err != nil) != tt.wantErr { t.Fatalf("ResolveResultRef() error = %v, wantErr %v", err, tt.wantErr) } @@ -362,6 +383,9 @@ func TestResolveResultRefs(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got, err := ResolveResultRefs(tt.args.pipelineRunState, tt.args.targets) + sort.SliceStable(got, func(i, j int) bool { + return strings.Compare(got[i].FromTaskRun, got[j].FromTaskRun) < 0 + }) if (err != nil) != tt.wantErr { t.Errorf("ResolveResultRefs() error = %v, wantErr %v", err, tt.wantErr) return