Skip to content

Commit

Permalink
Fix the variable substitution for task results inside array parameters
Browse files Browse the repository at this point in the history
  • Loading branch information
@othomann @tekton-robot
othomann authored and tekton-robot committed Mar 27, 2020
1 parent 2cdece9 commit 597f71c
Show file tree
Hide file tree
Showing 3 changed files with 394 additions and 320 deletions.
307 changes: 0 additions & 307 deletions pkg/apis/pipeline/v1alpha1/param_types_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (

"github.com/google/go-cmp/cmp"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
"github.com/tektoncd/pipeline/test/builder"
)

Expand Down Expand Up @@ -188,309 +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,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, _ := v1beta1.NewResultRefs(tt.args.param)
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,
},
}
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)
}
})
}
}
41 changes: 28 additions & 13 deletions pkg/apis/pipeline/v1beta1/param_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,9 +186,6 @@ func NewResultRefs(param Param) ([]*ResultRef, error) {
// This is useful if we want to make sure the param looks like a ResultReference before
// performing strict validation
func LooksLikeContainsResultRefs(param Param) bool {
if param.Value.Type != ParamTypeString {
return false
}
extractedExpressions, ok := getVarSubstitutionExpressions(param)
if !ok {
return false
Expand All @@ -207,18 +204,36 @@ func looksLikeResultRef(expression string) bool {

// getVarSubstitutionExpressions extracts all the value between "$(" and ")""
func getVarSubstitutionExpressions(param Param) ([]string, bool) {
if param.Value.Type != ParamTypeString {
return nil, false
}
expressions := variableSubstitutionRegex.FindAllString(param.Value.StringVal, -1)
if expressions == nil {
return nil, false
}
var allExpressions []string
for _, expression := range expressions {
allExpressions = append(allExpressions, stripVarSubExpression(expression))
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
}
return allExpressions, true
}

func stripVarSubExpression(expression string) string {
Expand Down
Loading

0 comments on commit 597f71c

Please sign in to comment.