Add support for using PipelineRun parameters as matrix parameter values

This commit adds support for using PipelineRun parameters as matrix parameter values by moving the validation for checking matrix parameter types to after parameter substitution. This is necessary for allowing whole array substitutions using the syntax $(params.foo[*]), rather than an explicit YAML array, to be used as a matrix parameter. Prior to this, validation fails because a string referencing an array type was considered a string type and this was done before the applying array replacements. Note: Whole array replacements for matrix results has not yet been implemented and will be added in a subsequent PR. This issue is described in more detail here: tektoncd#6056
EmmaMunley · Apr 28, 2023 · 226f961 · 226f961
1 parent dd9eea3
commit 226f961
Show file tree

Hide file tree

Showing 6 changed files with 110 additions and 84 deletions.
diff --git a/pkg/apis/pipeline/v1/matrix_types.go b/pkg/apis/pipeline/v1/matrix_types.go
@@ -15,8 +15,8 @@ package v1
 
 import (
 	"context"
-	"fmt"
 	"sort"
+	"strings"
 
 	"github.com/tektoncd/pipeline/pkg/apis/config"
 	"golang.org/x/exp/maps"

diff --git a/pkg/apis/pipeline/v1/pipeline_types_test.go b/pkg/apis/pipeline/v1/pipeline_types_test.go
@@ -626,65 +626,20 @@ func TestPipelineTask_ValidateMatrix(t *testing.T) {
 		},
 		wantErrs: apis.ErrMultipleOneOf("matrix[foobar]", "params[foobar]"),
 	}, {
-		name: "parameter duplicated in matrix.include.params and pipelinetask.params",
+		name: "parameters in matrix contain references to arrays",
 		pt: &PipelineTask{
 			Name: "task",
-			Matrix: &Matrix{
-				Include: IncludeParamsList{{
-					Name: "duplicate-param",
-					Params: Params{{
-						Name: "duplicate", Value: ParamValue{Type: ParamTypeString, StringVal: "foo"},
-					}}},
-				}},
 			Params: Params{{
-				Name: "duplicate", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}},
+				Name: "foobar", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}},
+			}, {
+				Name: "barfoo", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"bar", "foo"}},
 			}},
-		},
-		wantErrs: apis.ErrMultipleOneOf("matrix[duplicate]", "params[duplicate]"),
-	}, {
-		name: "duplicate parameters in matrix.params",
-		pt: &PipelineTask{
-			Name: "task",
 			Matrix: &Matrix{
 				Params: Params{{
-					Name: "foobar", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}},
+					Name: "foo", Value: ParamValue{Type: ParamTypeString, StringVal: "$(params.foobar[*])"},
 				}, {
-					Name: "foobar", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo-1", "bar-1"}},
-				}}},
-		},
-		wantErrs: &apis.FieldError{
-			Message: `parameter names must be unique, the parameter "foobar" is also defined at`,
-			Paths:   []string{"matrix.params[1].name"},
-		},
-	}, {
-		name: "parameters unique in matrix and params",
-		pt: &PipelineTask{
-			Name: "task",
-			Matrix: &Matrix{
-				Params: Params{{
-					Name: "foobar", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}},
+					Name: "bar", Value: ParamValue{Type: ParamTypeString, StringVal: "$(params.barfoo[*])"},
 				}}},
-			Params: Params{{
-				Name: "barfoo", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"bar", "foo"}},
-			}},
-		},
-	}, {
-		name: "duplicate parameters in matrix.include.params",
-		pt: &PipelineTask{
-			Name: "task",
-			Matrix: &Matrix{
-				Include: IncludeParamsList{{
-					Name: "invalid-include",
-					Params: Params{{
-						Name: "foobar", Value: ParamValue{Type: ParamTypeString, StringVal: "foo"},
-					}, {
-						Name: "foobar", Value: ParamValue{Type: ParamTypeString, StringVal: "foo-1"},
-					}}},
-				}},
-		},
-		wantErrs: &apis.FieldError{
-			Message: `parameter names must be unique, the parameter "foobar" is also defined at`,
-			Paths:   []string{"matrix.include[0].params[1].name"},
 		},
 	}, {
 		name: "parameters in matrix contain results references",

diff --git a/pkg/apis/pipeline/v1beta1/matrix_types.go b/pkg/apis/pipeline/v1beta1/matrix_types.go
@@ -15,7 +15,6 @@ package v1beta1
 
 import (
 	"context"
-	"fmt"
 	"sort"
 	"strings"
 

diff --git a/pkg/apis/pipeline/v1beta1/pipeline_types_test.go b/pkg/apis/pipeline/v1beta1/pipeline_types_test.go
@@ -615,49 +615,27 @@ func TestPipelineTask_validateMatrix(t *testing.T) {
 		},
 		wantErrs: apis.ErrMultipleOneOf("matrix[duplicate]", "params[duplicate]"),
 	}, {
-		name: "duplicate parameters in matrix.params",
-		pt: &PipelineTask{
-			Name: "task",
-			Matrix: &Matrix{
-				Params: Params{{
-					Name: "foobar", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}},
-				}, {
-					Name: "foobar", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo-1", "bar-1"}},
-				}}},
-		},
-		wantErrs: &apis.FieldError{
-			Message: `parameter names must be unique, the parameter "foobar" is also defined at`,
-			Paths:   []string{"matrix.params[1].name"},
-		},
-	}, {
-		name: "parameters unique in matrix and params",
+		name: "parameters in matrix contain references to arrays",
 		pt: &PipelineTask{
 			Name: "task",
-			Matrix: &Matrix{
-				Params: Params{{
-					Name: "foobar", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}},
-				}}},
 			Params: Params{{
+				Name: "foobar", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"foo", "bar"}},
+			}, {
 				Name: "barfoo", Value: ParamValue{Type: ParamTypeArray, ArrayVal: []string{"bar", "foo"}},
 			}},
 		},
 	}, {
-		name: "duplicate parameters in matrix.include.params",
+		name: "parameters in matrix contain whole array results references",
 		pt: &PipelineTask{
 			Name: "task",
 			Matrix: &Matrix{
-				Include: IncludeParamsList{{
-					Name: "invalid-include",
-					Params: Params{{
-						Name: "foobar", Value: ParamValue{Type: ParamTypeString, StringVal: "foo"},
-					}, {
-						Name: "foobar", Value: ParamValue{Type: ParamTypeString, StringVal: "foo-1"},
-					}}},
-				}},
+				Params: Params{{
+					Name: "a-param", Value: ParamValue{Type: ParamTypeString, StringVal: "$(tasks.foo-task.results.[*])"},
+				}}},
 		},
 		wantErrs: &apis.FieldError{
-			Message: `parameter names must be unique, the parameter "foobar" is also defined at`,
-			Paths:   []string{"matrix.include[0].params[1].name"},
+			Message: "matrix parameters cannot whole array result references",
+			Paths:   []string{"matrix.params[0]"},
 		},
 	}, {
 		name: "parameters in matrix contain results references",

diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go
@@ -6643,6 +6643,99 @@ spec:
 	}
 }
 
+func TestReconcile_ParamsValidationsImmediatelyFailPipelineRun(t *testing.T) {
+	names.TestingSeed()
+
+	ctx := context.Background()
+	cfg := config.NewStore(logtesting.TestLogger(t))
+	ctx = cfg.ToContext(ctx)
+
+	prs := []*v1beta1.PipelineRun{
+		parse.MustParseV1beta1PipelineRun(t, `
+metadata:
+  name: pipelinerun-matrix-param-invalid-type
+  namespace: foo
+spec:
+  pipelineSpec:
+   tasks:
+    - name: mytask
+      taskSpec:
+        steps:
+         - name: echo
+           image: alpine
+           script: |
+             echo "test"
+    - name: platforms-and-browsers
+      taskRef:
+        name: mytask
+      matrix:
+        params:
+          - name: platform
+            value: linux
+          - name: browser
+            value:
+              - chrome
+              - safari
+              - firefox
+      params:
+        - name: version
+          value: v0.33.0
+`),
+		parse.MustParseV1beta1PipelineRun(t, `
+metadata:
+  name: pipelinerun-matrix-param-duplicates
+  namespace: foo
+spec:
+  pipelineSpec:
+   tasks:
+    - name: mytask
+      matrix:
+        params:
+          - name: browser
+            value:
+              - chrome
+          - name: browser
+            value:
+              - safari
+              - firefox
+      taskSpec:
+        steps:
+         - name: echo
+           image: alpine
+           script: |
+             echo "test"
+`),
+	}
+
+	cms := []*corev1.ConfigMap{withEnabledAlphaAPIFields(newFeatureFlagsConfigMap())}
+	d := test.Data{
+		PipelineRuns: prs,
+		ConfigMaps:   cms,
+	}
+	prt := newPipelineRunTest(t, d)
+	defer prt.Cancel()
+
+	run1, _ := prt.reconcileRun("foo", "pipelinerun-matrix-param-invalid-type", nil, true)
+	run2, _ := prt.reconcileRun("foo", "pipelinerun-matrix-param-duplicates", nil, true)
+
+	cond1 := run1.Status.GetCondition(apis.ConditionSucceeded)
+	cond2 := run2.Status.GetCondition(apis.ConditionSucceeded)
+
+	for _, c := range []*apis.Condition{cond1, cond2} {
+		if c.Status != corev1.ConditionFalse {
+			t.Errorf("expected Succeeded/False condition but saw: %v", c)
+		}
+	}
+
+	if cond1.Reason != ReasonInvalidMatrixParameterTypes {
+		t.Errorf("expected invalid matrix parameter types condition but saw: %v", cond1)
+	}
+
+	if cond2.Reason != ReasonDuplicateParams {
+		t.Errorf("expected invalid duplicate parameters condition but saw: %v", cond2)
+	}
+}
+
 // TestReconcileWithResolver checks that a PipelineRun with a populated Resolver
 // field creates a ResolutionRequest object for that Resolver's type, and
 // that when the request is successfully resolved the PipelineRun begins running.

diff --git a/pkg/reconciler/pipelinerun/resources/validate_params.go b/pkg/reconciler/pipelinerun/resources/validate_params.go
@@ -22,6 +22,7 @@ import (
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
 	"github.com/tektoncd/pipeline/pkg/list"
 	"github.com/tektoncd/pipeline/pkg/reconciler/taskrun"
+	"k8s.io/apimachinery/pkg/util/sets"
 )
 
 // ValidateParamTypesMatching validate that parameters in PipelineRun override corresponding parameters in Pipeline of the same type.
-Original file line number
+Diff line change
@@ Expand Up / @@ -15,7 +15,6 @@ package v1beta1 @@
     import (
     	"context"
-    	"fmt"
     	"sort"
     	"strings"
@@ Expand Down @@