From b4f27e03a09869b204eaa50230b80cc2750a0e1e 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 | 393 +++++++++++++++++- pkg/reconciler/pipelinerun/resources/apply.go | 2 +- .../pipelinerun/resources/validate_params.go | 6 +- .../resources/validate_params_test.go | 6 +- 6 files changed, 441 insertions(+), 8 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..bfc201c7e24 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", "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 @@ -12176,6 +12224,349 @@ spec: } } +func TestReconciler_PipelineTaskMatrix_OnError(t *testing.T) { + names.TestingSeed() + + task := parse.MustParseV1Task(t, ` +metadata: + name: mytask + namespace: foo +spec: + params: + - name: param-1 + steps: + - name: echo + onError: continue + image: alpine + script: exit 1 +`) + + expectedTaskRuns := []*v1.TaskRun{ + mustParseTaskRunWithObjectMeta(t, + taskRunObjectMeta("pr-matrix-with-onerror-0", "foo", + "pr", "p", "matrix-with-onerror", false), + ` +spec: + params: + - name: param-1 + value: "0" + serviceAccountName: test-sa + taskRef: + name: mytask + kind: Task +`), + mustParseTaskRunWithObjectMeta(t, + taskRunObjectMeta("pr-matrix-with-onerror-1", "foo", + "pr", "p", "matrix-with-onerror", false), + ` +spec: + params: + - name: param-1 + value: "1" + serviceAccountName: test-sa + taskRef: + name: mytask + kind: Task +`), + mustParseTaskRunWithObjectMeta(t, + taskRunObjectMeta("pr-matrix-with-onerror-2", "foo", + "pr", "p", "matrix-with-onerror", false), + ` +spec: + params: + - name: param-1 + value: "2" + serviceAccountName: test-sa + taskRef: + name: mytask + kind: Task +`), + mustParseTaskRunWithObjectMeta(t, + taskRunObjectMeta("pr-matrix-with-onerror-3", "foo", + "pr", "p", "matrix-with-onerror", false), + ` +spec: + params: + - name: param-1 + value: "3" + serviceAccountName: test-sa + taskRef: + name: mytask + kind: Task +`), + } + cms := []*corev1.ConfigMap{withEnabledAlphaAPIFields(newFeatureFlagsConfigMap())} + cms = append(cms, withMaxMatrixCombinationsCount(newDefaultsConfigMap(), 10)) + + tests := []struct { + name string + memberOf string + p *v1.Pipeline + tr *v1.TaskRun + expectedPipelineRun *v1.PipelineRun + }{{ + name: "p-dag", + memberOf: "tasks", + p: parse.MustParseV1Pipeline(t, fmt.Sprintf(` +metadata: + name: %s + namespace: foo +spec: + tasks: + - name: matrix-with-onerror + taskRef: + name: mytask + matrix: + params: + - name: param-1 + value: + - "0" + - "1" + - "2" + - "3" +`, "p-dag")), + expectedPipelineRun: parse.MustParseV1PipelineRun(t, ` +metadata: + name: pr + namespace: foo + annotations: {} + labels: + tekton.dev/pipeline: p-dag +spec: + taskRunTemplate: + serviceAccountName: test-sa + pipelineRef: + name: p-dag +status: + pipelineSpec: + tasks: + - name: matrix-with-onerror + taskRef: + name: mytask + kind: Task + matrix: + params: + - name: param-1 + value: + - "0" + - "1" + - "2" + - "3" + conditions: + - type: Succeeded + status: "Unknown" + reason: "Running" + message: "Tasks Completed: 0 (Failed: 0, Cancelled 0), Incomplete: 1, Skipped: 0" + childReferences: + - apiVersion: tekton.dev/v1 + kind: TaskRun + name: pr-matrix-with-onerror-0 + pipelineTaskName: matrix-with-onerror + - apiVersion: tekton.dev/v1 + kind: TaskRun + name: pr-matrix-with-onerror-1 + pipelineTaskName: matrix-with-onerror + - apiVersion: tekton.dev/v1 + kind: TaskRun + name: pr-matrix-with-onerror-2 + pipelineTaskName: matrix-with-onerror + - apiVersion: tekton.dev/v1 + kind: TaskRun + name: pr-matrix-with-onerror-3 + pipelineTaskName: matrix-with-onerror + provenance: + featureFlags: + RunningInEnvWithInjectedSidecars: true + EnableTektonOCIBundles: true + EnableAPIFields: "alpha" + EnforceNonfalsifiability: "none" + AwaitSidecarReadiness: true + VerificationNoMatchPolicy: "ignore" + EnableProvenanceInStatus: true + ResultExtractionMethod: "termination-message" + MaxResultSize: 4096 + Coschedule: "workspaces" +`), + }, { + name: "p-finally", + memberOf: "finally", + p: parse.MustParseV1Pipeline(t, fmt.Sprintf(` +metadata: + name: %s + namespace: foo +spec: + tasks: + - name: unmatrixed-pt + params: + - name: param-1 + value: "100" + taskRef: + name: mytask + finally: + - name: matrix-with-onerror + taskRef: + name: mytask + matrix: + params: + - name: param-1 + value: + - "0" + - "1" + - "2" + - "3" +`, "p-finally")), + tr: mustParseTaskRunWithObjectMeta(t, + taskRunObjectMeta("pr-unmatrixed-pt", "foo", + "pr", "p-finally", "unmatrixed-pt", false), + ` +spec: + params: + - name: param-1 + value: "100" + serviceAccountName: test-sa + taskRef: + name: mytask + kind: Task +status: + conditions: + - type: Succeeded + status: "True" + reason: Succeeded + message: All Tasks have completed executing +`), + expectedPipelineRun: parse.MustParseV1PipelineRun(t, ` +metadata: + name: pr + namespace: foo + annotations: {} + labels: + tekton.dev/pipeline: p-finally +spec: + taskRunTemplate: + serviceAccountName: test-sa + pipelineRef: + name: p-finally +status: + pipelineSpec: + tasks: + - name: unmatrixed-pt + params: + - name: param-1 + value: "100" + taskRef: + name: mytask + kind: Task + finally: + - name: matrix-with-onerror + taskRef: + name: mytask + kind: Task + matrix: + params: + - name: param-1 + value: + - "0" + - "1" + - "2" + - "3" + conditions: + - type: Succeeded + status: "Unknown" + reason: "Running" + message: "Tasks Completed: 1 (Failed: 0, Cancelled 0), Incomplete: 1, Skipped: 0" + childReferences: + - apiVersion: tekton.dev/v1 + kind: TaskRun + name: pr-unmatrixed-pt + pipelineTaskName: unmatrixed-pt + - apiVersion: tekton.dev/v1 + kind: TaskRun + name: pr-matrix-with-onerror-0 + pipelineTaskName: matrix-with-onerror + - apiVersion: tekton.dev/v1 + kind: TaskRun + name: pr-matrix-with-onerror-1 + pipelineTaskName: matrix-with-onerror + - apiVersion: tekton.dev/v1 + kind: TaskRun + name: pr-matrix-with-onerror-2 + pipelineTaskName: matrix-with-onerror + - apiVersion: tekton.dev/v1 + kind: TaskRun + name: pr-matrix-with-onerror-3 + pipelineTaskName: matrix-with-onerror + provenance: + featureFlags: + RunningInEnvWithInjectedSidecars: true + EnableTektonOCIBundles: true + EnableAPIFields: "alpha" + EnforceNonfalsifiability: "none" + AwaitSidecarReadiness: true + VerificationNoMatchPolicy: "ignore" + EnableProvenanceInStatus: true + ResultExtractionMethod: "termination-message" + MaxResultSize: 4096 + Coschedule: "workspaces" + +`), + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pr := parse.MustParseV1PipelineRun(t, fmt.Sprintf(` +metadata: + name: pr + namespace: foo +spec: + taskRunTemplate: + serviceAccountName: test-sa + pipelineRef: + name: %s +`, tt.name)) + d := test.Data{ + PipelineRuns: []*v1.PipelineRun{pr}, + Pipelines: []*v1.Pipeline{tt.p}, + Tasks: []*v1.Task{task}, + ConfigMaps: cms, + } + if tt.tr != nil { + d.TaskRuns = []*v1.TaskRun{tt.tr} + } + prt := newPipelineRunTest(t, d) + defer prt.Cancel() + + _, clients := prt.reconcileRun("foo", "pr", []string{}, false) + taskRuns, err := clients.Pipeline.TektonV1().TaskRuns("foo").List(prt.TestAssets.Ctx, metav1.ListOptions{ + LabelSelector: fmt.Sprintf("tekton.dev/pipelineRun=pr,tekton.dev/pipeline=%s,tekton.dev/pipelineTask=matrix-with-onerror", tt.name), + Limit: 1, + }) + if err != nil { + t.Fatalf("Failure to list TaskRun's %s", err) + } + + if len(taskRuns.Items) != 4 { + t.Fatalf("Expected 4 TaskRuns got %d", len(taskRuns.Items)) + } + + for i := range taskRuns.Items { + expectedTaskRun := expectedTaskRuns[i] + expectedTaskRun.Labels["tekton.dev/pipeline"] = tt.name + expectedTaskRun.Labels["tekton.dev/memberOf"] = tt.memberOf + if d := cmp.Diff(expectedTaskRun, &taskRuns.Items[i], ignoreResourceVersion, ignoreTypeMeta); d != "" { + t.Errorf("expected to see TaskRun %v created. Diff %s", expectedTaskRuns[i].Name, diff.PrintWantGot(d)) + } + } + + pipelineRun, err := clients.Pipeline.TektonV1().PipelineRuns("foo").Get(prt.TestAssets.Ctx, "pr", metav1.GetOptions{}) + if err != nil { + t.Fatalf("Got an error getting reconciled run out of fake client: %s", err) + } + if d := cmp.Diff(tt.expectedPipelineRun, pipelineRun, ignoreResourceVersion, ignoreTypeMeta, ignoreLastTransitionTime, ignoreStartTime, ignoreFinallyStartTime, cmpopts.EquateEmpty()); d != "" { + t.Errorf("expected PipelineRun was not created. Diff %s", diff.PrintWantGot(d)) + } + }) + } +} + func TestReconcile_SetDefaults(t *testing.T) { names.TestingSeed() 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)