From b70ea8aeff7b311cd75e31faa5c14a13b50fc333 Mon Sep 17 00:00:00 2001 From: Priti Desai Date: Wed, 23 Aug 2023 14:08:07 -0700 Subject: [PATCH] updating example of a matrix feature Fixing a bug with InvalidMatrixParameterType when a parameter is not part of a matrix but defined as a task parameter of type string, pipelineRun results in a failure. Adding two new validation: * onError with matrix: when a task fan out for a given number of instances, pipeline controller must apply the onError to each running instance. * retry with matrix: when a task has specified retry, that specification must apply to all the instances of fanned out task. Being consistent with the rest of the reasons, dropping "Reason" from "ReasonInvalidMatrixParameterTypes". Updated the error reported in "ValidateParameterTypesInMatrix" to include name of the pipelineTask. Co-authored-by: EmmaMunley Signed-off-by: Priti Desai --- .../alpha/pipelinerun-with-matrix.yaml | 40 ++++++++++++++ pkg/reconciler/pipelinerun/pipelinerun.go | 2 +- .../pipelinerun/pipelinerun_test.go | 52 ++++++++++++++++++- pkg/reconciler/pipelinerun/resources/apply.go | 2 +- .../pipelinerun/resources/validate_params.go | 6 ++- .../resources/validate_params_test.go | 6 +-- 6 files changed, 99 insertions(+), 9 deletions(-) diff --git a/examples/v1/pipelineruns/alpha/pipelinerun-with-matrix.yaml b/examples/v1/pipelineruns/alpha/pipelinerun-with-matrix.yaml index 6105430834d..c9a70f9c121 100644 --- a/examples/v1/pipelineruns/alpha/pipelinerun-with-matrix.yaml +++ b/examples/v1/pipelineruns/alpha/pipelinerun-with-matrix.yaml @@ -64,6 +64,46 @@ spec: - name: echo image: ubuntu script: exit 1 + - name: matrix-with-task-using-onerror + matrix: + params: + - name: version + value: + - "1" + - "2" + taskSpec: + params: + - name: version + steps: + - name: echo + image: ubuntu + onError: continue + script: exit 1 + - name: matrix-with-task-retries + retries: 1 + params: + - name: pipelineTask-retries + value: $(context.pipelineTask.retries) + matrix: + params: + - name: version + value: + - "1" + - "2" + taskSpec: + params: + - name: version + - name: pipelineTask-retries + steps: + - image: alpine + script: | + #!/usr/bin/env sh + if [ "$(context.task.retry-count)" == "$(params.pipelineTask-retries)" ]; then + echo "This is the last retry." + exit 0; + fi + echo "The PipelineTask has retried $(context.task.retry-count) times." + exit 1 finally: - name: matrix-params-with-empty-array-skipped-in-finally matrix: diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 760b8b9cb9c..e5f10129ef0 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -121,7 +121,7 @@ const ( // all of the running TaskRuns as timed out failed. ReasonCouldntTimeOut = "PipelineRunCouldntTimeOut" // ReasonInvalidMatrixParameterTypes indicates a matrix contains invalid parameter types - ReasonInvalidMatrixParameterTypes = "ReasonInvalidMatrixParameterTypes" + ReasonInvalidMatrixParameterTypes = "InvalidMatrixParameterTypes" // ReasonInvalidTaskResultReference indicates a task result was declared // but was not initialized by that task ReasonInvalidTaskResultReference = "InvalidTaskResultReference" diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index d02bb8264d3..391d3a5016b 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -8228,6 +8228,23 @@ spec: name: mytask kind: Task `), + mustParseTaskRunWithObjectMeta(t, + taskRunObjectMeta("pr-matrix-using-platforms-0", "foo", + "pr", "p", "matrix-using-platforms", false), + ` +spec: + params: + - name: browser + value: chrome + - name: platform + value: linux + - name: version + value: v0.33.0 + serviceAccountName: test-sa + taskRef: + name: mytask + kind: Task + `), } cms := []*corev1.ConfigMap{withEnabledAlphaAPIFields(newFeatureFlagsConfigMap())} cms = append(cms, withMaxMatrixCombinationsCount(newDefaultsConfigMap(), 10)) @@ -8265,6 +8282,19 @@ spec: params: - name: version value: v0.33.0 + - name: matrix-using-platforms + taskRef: + name: mytask + matrix: + params: + - name: platform + value: + - linux + params: + - name: browser + value: chrome + - name: version + value: v0.33.0 `, "p-dag")), expectedPipelineRun: parse.MustParseV1PipelineRun(t, ` metadata: @@ -8300,11 +8330,25 @@ status: params: - name: version value: v0.33.0 + - name: matrix-using-platforms + taskRef: + name: mytask + kind: Task + matrix: + params: + - name: platform + value: + - linux + params: + - name: browser + value: chrome + - name: version + value: v0.33.0 conditions: - type: Succeeded status: "Unknown" reason: "Running" - message: "Tasks Completed: 0 (Failed: 0, Cancelled 0), Incomplete: 1, Skipped: 0" + message: "Tasks Completed: 0 (Failed: 0, Cancelled 0), Incomplete: 2, Skipped: 0" childReferences: - apiVersion: tekton.dev/v1 kind: TaskRun @@ -8342,6 +8386,10 @@ status: kind: TaskRun name: pr-platforms-and-browsers-8 pipelineTaskName: platforms-and-browsers + - apiVersion: tekton.dev/v1 + kind: TaskRun + name: pr-matrix-using-platforms + pipelineTaskName: matrix-using-platforms provenance: featureFlags: RunningInEnvWithInjectedSidecars: true @@ -8558,7 +8606,7 @@ spec: } if len(taskRuns.Items) != 9 { - t.Fatalf("Expected 9 TaskRuns got %d", len(taskRuns.Items)) + t.Fatalf("Expected 10 TaskRuns got %d", len(taskRuns.Items)) } for i := range taskRuns.Items { diff --git a/pkg/reconciler/pipelinerun/resources/apply.go b/pkg/reconciler/pipelinerun/resources/apply.go index 4aebc6450df..63171581f2c 100644 --- a/pkg/reconciler/pipelinerun/resources/apply.go +++ b/pkg/reconciler/pipelinerun/resources/apply.go @@ -159,7 +159,7 @@ func ApplyPipelineTaskContexts(pt *v1.PipelineTask) *v1.PipelineTask { } pt.Params = pt.Params.ReplaceVariables(replacements, map[string][]string{}, map[string]map[string]string{}) if pt.IsMatrixed() { - pt.Matrix.Params = pt.Params.ReplaceVariables(replacements, map[string][]string{}, map[string]map[string]string{}) + pt.Matrix.Params = pt.Matrix.Params.ReplaceVariables(replacements, map[string][]string{}, map[string]map[string]string{}) for i := range pt.Matrix.Include { pt.Matrix.Include[i].Params = pt.Matrix.Include[i].Params.ReplaceVariables(replacements, map[string][]string{}, map[string]map[string]string{}) } diff --git a/pkg/reconciler/pipelinerun/resources/validate_params.go b/pkg/reconciler/pipelinerun/resources/validate_params.go index f3a4a8c7749..d954756f1d0 100644 --- a/pkg/reconciler/pipelinerun/resources/validate_params.go +++ b/pkg/reconciler/pipelinerun/resources/validate_params.go @@ -96,7 +96,8 @@ func ValidateParameterTypesInMatrix(state PipelineRunState) error { for _, include := range m.Include { for _, param := range include.Params { if param.Value.Type != v1.ParamTypeString { - return fmt.Errorf("parameters of type string only are allowed, but param %s has type %s", param.Name, string(param.Value.Type)) + return fmt.Errorf("parameters of type string only are allowed, but param \"%s\" has type \"%s\" in pipelineTask \"%s\"", + param.Name, string(param.Value.Type), rpt.PipelineTask.Name) } } } @@ -104,7 +105,8 @@ func ValidateParameterTypesInMatrix(state PipelineRunState) error { if m.HasParams() { for _, param := range m.Params { if param.Value.Type != v1.ParamTypeArray { - return fmt.Errorf("parameters of type array only are allowed, but param %s has type %s", param.Name, string(param.Value.Type)) + return fmt.Errorf("parameters of type array only are allowed, but param \"%s\" has type \"%s\" in pipelineTask \"%s\"", + param.Name, string(param.Value.Type), rpt.PipelineTask.Name) } } } diff --git a/pkg/reconciler/pipelinerun/resources/validate_params_test.go b/pkg/reconciler/pipelinerun/resources/validate_params_test.go index 69fa1f7a1eb..fefce539c7e 100644 --- a/pkg/reconciler/pipelinerun/resources/validate_params_test.go +++ b/pkg/reconciler/pipelinerun/resources/validate_params_test.go @@ -355,7 +355,7 @@ func TestValidatePipelineParameterTypes(t *testing.T) { }}}, }, }}, - wantErrs: "parameters of type array only are allowed, but param foo has type string", + wantErrs: "parameters of type array only are allowed, but param \"foo\" has type \"string\" in pipelineTask \"task\"", }, { desc: "parameters in include matrix are strings", state: resources.PipelineRunState{{ @@ -386,7 +386,7 @@ func TestValidatePipelineParameterTypes(t *testing.T) { }}}, }, }}, - wantErrs: "parameters of type string only are allowed, but param foobar has type array", + wantErrs: "parameters of type string only are allowed, but param \"foobar\" has type \"array\" in pipelineTask \"task\"", }, { desc: "parameters in include matrix are objects", state: resources.PipelineRunState{{ @@ -409,7 +409,7 @@ func TestValidatePipelineParameterTypes(t *testing.T) { }}}, }, }}, - wantErrs: "parameters of type string only are allowed, but param barfoo has type object", + wantErrs: "parameters of type string only are allowed, but param \"barfoo\" has type \"object\" in pipelineTask \"task\"", }} { t.Run(tc.desc, func(t *testing.T) { err := resources.ValidateParameterTypesInMatrix(tc.state)