fix an issue with `InvalidMatrixParameterTypes` along with updating the matrix example with additional validations #7064

pritidesai · 2023-08-23T21:18:52Z

Changes

Fixing an issue where the ApplyPipelineTaskContexts had bug in ReplaceVariables with replacing matrix.params with just params instead of matrix.params
Adding two new validations:
- 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 emmamunley@google.com

Closes #7070

/kind bug

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

examples/v1/pipelineruns/alpha/pipelinerun-with-matrix.yaml

pritidesai · 2023-08-24T16:48:43Z

@pritidesai: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-tekton-pipeline-alpha-integration-tests ed441e7 link true /test pull-tekton-pipeline-alpha-integration-tests
Full PR test history. Your PR dashboard.

This is not a flake :(

     status:
          completionTime: "2023-08-24T05:03:18Z"
          conditions:
          - lastTransitionTime: "2023-08-24T05:03:18Z"
            message: parameters of type array only are allowed, but param "browser" has type
              "string" in pipelineTask "matrix-and-params"
            reason: InvalidMatrixParameterTypes
            status: "False"
            type: Succeeded

The pipelineTask matrix-and-params does not have a param named browser as part of a matrix.

https://github.com/tektoncd/pipeline/pull/7064/files#diff-92ed82e9a2fce3f31e0727fba14c4d9d8a2900c039728e5ae467d2a76bb35a6aR42

This pipelineTask is not updated in this PR. This PR is adding two additional pipelineTasks in the same pipeline.

Why is browser listed as a matrix param 🤔

https://github.com/tektoncd/pipeline/pull/7064/files#diff-7bab0e0c817e47606ac9e254920284dbb34735b3bec9a6ecbe9852ced5f9795fR105-R109

pritidesai · 2023-08-30T18:32:10Z

/kind bug

EmmaMunley · 2023-09-01T16:42:18Z

pkg/reconciler/pipelinerun/pipelinerun_test.go

@@ -8558,7 +8606,7 @@ spec:
 			}

 			if len(taskRuns.Items) != 9 {


Suggested change

if len(taskRuns.Items) != 9 {

if len(taskRuns.Items) != 10 {

Should this be updated to 10?

taskRuns is retrieving a list of taskRuns created for a matrix pipelineTask platforms-and-browsers and expected that the reconciler creates 9 taskRuns. I have reverted the changes to the error message to reflect the right number.

If needed, we can change this a little bit more to also retrieve taskRun for the other matrix pipelineTask and make sure it only creates one taskRun.

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 <emmamunley@google.com> Signed-off-by: Priti Desai <pdesai@us.ibm.com>

Yongxuanzhang · 2023-09-01T18:54:43Z

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{})


do we have unit tests in apply_test.go to cover this?

The existing unit tests does cover this.

But the existing test was not able to catch this issue. This bug surfaces when a non-matrix parameter (which can be of any type) is added to the list of params belonging to a matrix (only arrays are allowed). We have added a test to catch this error (parameters of type array only are allowed, but param version has type string) and address it as part of this PR.

If we want to still add a test local unit test, we could add the following test case. I think this is not necessary. Let me know you think.

description: "matrix params are not overwritten by the params", pt: v1.PipelineTask{ Params: v1.Params{{ Name: "params-1", Value: *v1.NewStructuredValues("0"), }}, Matrix: &v1.Matrix{ Params: v1.Params{{ Name: "params-2", Value: *v1.NewStructuredValues("5"), }}, }, }, want: v1.PipelineTask{ Params: v1.Params{{ Name: "params-1", Value: *v1.NewStructuredValues("0"), }}, Matrix: &v1.Matrix{ Params: v1.Params{{ Name: "params-2", Value: *v1.NewStructuredValues("5"), }}, },

dibyom · 2023-09-01T18:58:33Z

pkg/reconciler/pipelinerun/pipelinerun_test.go

+    kind: TaskRun
+    name: pr-matrix-with-onerror-3
+    pipelineTaskName: matrix-with-onerror
+  provenance:


non-blocking: I wonder if it makes sense to ignore the provenance fields for these tests

sure @dibyom I will open a separate PR to clean up provenance fields as its part of many other tests.

jerop

/lgtm

thank you @pritidesai!

tekton-robot · 2023-09-05T13:51:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom, jerop

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dibyom,jerop]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. kind/misc Categorizes issue or PR as a miscellaneuous one. labels Aug 23, 2023

tekton-robot requested review from afrittoli and jerop August 23, 2023 21:18

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 23, 2023

pritidesai force-pushed the matrix-examples branch from d852d1d to ed441e7 Compare August 24, 2023 04:30

QuanZhang-William reviewed Aug 24, 2023

View reviewed changes

examples/v1/pipelineruns/alpha/pipelinerun-with-matrix.yaml Show resolved Hide resolved

pritidesai mentioned this pull request Aug 24, 2023

InvalidMatrixParameterTypes for param which is not part of matrix #7070

Closed

pritidesai force-pushed the matrix-examples branch from ed441e7 to d0e7f68 Compare August 30, 2023 18:31

pritidesai removed the kind/misc Categorizes issue or PR as a miscellaneuous one. label Aug 30, 2023

tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 30, 2023

pritidesai changed the title ~~updating matrix example with additional validations~~ fix an issue with InvalidMatrixParameterTypes along with updating the matrix example with additional validations Aug 30, 2023

pritidesai changed the title ~~fix an issue with InvalidMatrixParameterTypes along with updating the matrix example with additional validations~~ fix an issue with InvalidMatrixParameterTypes along with updating the matrix example with additional validations Aug 30, 2023

pritidesai force-pushed the matrix-examples branch from d0e7f68 to b70ea8a Compare September 1, 2023 01:32

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 1, 2023

pritidesai force-pushed the matrix-examples branch from b70ea8a to 06faa10 Compare September 1, 2023 05:28

pritidesai added this to the Pipelines v0.52 milestone Sep 1, 2023

EmmaMunley reviewed Sep 1, 2023

View reviewed changes

pritidesai force-pushed the matrix-examples branch from 06faa10 to b4f27e0 Compare September 1, 2023 17:24

Yongxuanzhang reviewed Sep 1, 2023

View reviewed changes

dibyom reviewed Sep 1, 2023

View reviewed changes

dibyom approved these changes Sep 1, 2023

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 1, 2023

jerop approved these changes Sep 5, 2023

View reviewed changes

tekton-robot assigned jerop Sep 5, 2023

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 5, 2023

tekton-robot merged commit c887347 into tektoncd:main Sep 5, 2023
2 checks passed

pritidesai deleted the matrix-examples branch September 5, 2023 15:30

pritidesai mentioned this pull request Sep 5, 2023

cleaning up provenance from pipelineRun #7091

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix an issue with `InvalidMatrixParameterTypes` along with updating the matrix example with additional validations #7064

fix an issue with `InvalidMatrixParameterTypes` along with updating the matrix example with additional validations #7064

pritidesai commented Aug 23, 2023 •

edited

Loading

pritidesai commented Aug 24, 2023 •

edited

Loading

pritidesai commented Aug 30, 2023

EmmaMunley Sep 1, 2023

pritidesai Sep 1, 2023

Yongxuanzhang Sep 1, 2023

pritidesai Sep 1, 2023 •

edited

Loading

dibyom Sep 1, 2023

pritidesai Sep 1, 2023

jerop left a comment

tekton-robot commented Sep 5, 2023

fix an issue with InvalidMatrixParameterTypes along with updating the matrix example with additional validations #7064

fix an issue with InvalidMatrixParameterTypes along with updating the matrix example with additional validations #7064

Conversation

pritidesai commented Aug 23, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

pritidesai commented Aug 24, 2023 • edited Loading

pritidesai commented Aug 30, 2023

EmmaMunley Sep 1, 2023

Choose a reason for hiding this comment

pritidesai Sep 1, 2023

Choose a reason for hiding this comment

Yongxuanzhang Sep 1, 2023

Choose a reason for hiding this comment

pritidesai Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

dibyom Sep 1, 2023

Choose a reason for hiding this comment

pritidesai Sep 1, 2023

Choose a reason for hiding this comment

jerop left a comment

Choose a reason for hiding this comment

tekton-robot commented Sep 5, 2023

fix an issue with `InvalidMatrixParameterTypes` along with updating the matrix example with additional validations #7064

fix an issue with `InvalidMatrixParameterTypes` along with updating the matrix example with additional validations #7064

pritidesai commented Aug 23, 2023 •

edited

Loading

pritidesai commented Aug 24, 2023 •

edited

Loading

pritidesai Sep 1, 2023 •

edited

Loading