TEP-0090: Include `Matrix` in Validation of `Parameters` in `TaskRun` Reconciler #4844

jerop · 2022-05-06T20:36:44Z

Changes

The TaskRun reconciler validates that the needed Parameters are provided,
no extra Parameters are provide and the types of Parameters are matching.

Prior to this change, the reconciler did the above validation considering the
params field in the PipelineTask only. In this change, we include matrix
field into the validation routine described above. This includes validating
that Parameters in the Matrix are substituting Parameters of type String
in the underlying Task.

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

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Release notes block below has been filled in (if there are no user facing changes, use release note "NONE")

Release Notes

NONE

lbernick · 2022-05-09T13:04:34Z

pkg/reconciler/taskrun/validate_resources.go

@@ -141,12 +141,26 @@ func wrongTypeParamsNames(params []v1beta1.Param, neededParamsTypes map[string]v
 			wrongTypeParamNames = append(wrongTypeParamNames, param.Name)
 		}
 	}
+	for _, param := range matrix {


It feels a bit strange to have code in reconciler/taskrun validating that matrix params aren't array params-- ideally the taskrun reconciler shouldn't have knowledge of matrix at all. could this validation be done at the pipelinerun level, and treat the matrix params as normal params by the time they get to the taskrun level? Or is it necessary to ensure that params passed via the matrix aren't declared as array params in the task?

My mental model of how this would work (based on tep 90) is that the params are declared as arrays in the pipeline, passed as matrix params in the pipeline task, and then passed from the pipeline task to the taskrun as a list of string params. If the params were declared as array params in the task, the taskrun reconciler would recognize that the string param passed didn't match the array param declared, and fail.

added that check out of an abundance of caution, but it's not necessary as that's already covered in the pipelinerun reconciler - #4704 - so I've removed it

Rather than pass in matrix params separately, any reason to not just include them in the existing slice of params to validate?

Hmmm... a bit confused since the above comment mentions that the check for param types was removed but I still see it below. Do we still need to resolve that?

Rather than pass in matrix params separately, any reason to not just include them in the existing slice of params to validate?

@lbernick - The existing slice of params is checking if the type of params provided in the pipeline matches the type of params in taskspec (string to string, and array to array). However, we want the type of params provided in the matrix to be array and the type of params they are substituting in the taskspec to be string. Because the types do not match for matrix params, we can't pass it to the existing validation and that's why we have its own validation. Does that clarify?

Hmmm... a bit confused since the above comment mentions that the check for param types was removed but I still see it below. Do we still need to resolve that?

@dibyom - Removed the validation that checks that the param in a matrix is of type array - this is already handled in the pipelinerun reconciler which was implemented in #4704. We still need to validate that the param that's being substituted by the param in a matrix is of type string in the taskrun reconciler - this is the type checking that we still have here.

Got it thanks Jerop!

pkg/reconciler/pipelinerun/pipelinerun.go

jerop · 2022-05-09T14:56:58Z

/retest

jerop · 2022-05-09T16:12:14Z

/retest

jerop · 2022-05-09T17:51:36Z

/test pull-tekton-pipeline-go-coverage

tekton-robot · 2022-05-09T17:53:48Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/taskrun/validate_resources.go	96.0%	94.3%	-1.7

… Reconciler The `TaskRun` reconciler validates that the needed `Parameters` are provided, no extra `Parameters` are provide and the types of `Parameters` are matching. Prior to this change, the reconciler did the above validation considering the `params` field in the `PipelineTask` only. In this change, we include `matrix` field into the validation routine described above. This includes validating that `Parameters` in the `Matrix` are substituting `Parameters` of type `String` in the underlying `Task`. Related: - tektoncd#4704 - tektoncd#4841

jerop · 2022-05-09T20:19:38Z

/test pull-tekton-pipeline-go-coverage

tekton-robot · 2022-05-09T20:21:48Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/taskrun/validate_resources.go	96.0%	95.3%	-0.8

tekton-robot · 2022-05-09T20:39:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

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]

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

dibyom · 2022-05-09T20:40:32Z

I unresolved a thread because I wasn't sure it was fully resolved but otherwise this looks good to me!

jerop · 2022-05-09T22:03:32Z

I unresolved a thread because I wasn't sure it was fully resolved but otherwise this looks good to me!

@dibyom - I'd not seen the follow up questions, addressed them, thank you!

abayer · 2022-05-10T14:26:15Z

/lgtm

jerop · 2022-05-10T14:26:36Z

/hold

dibyom · 2022-05-10T15:07:26Z

/lgtm

jerop · 2022-05-10T15:30:42Z

/hold cancel

jerop · 2022-05-10T15:42:05Z

/test pull-pipeline-kind-k8s-v1-21-e2e

jerop · 2022-05-10T18:54:22Z

/test pull-tekton-pipeline-alpha-integration-tests

jerop · 2022-05-10T20:19:22Z

/test pull-tekton-pipeline-alpha-integration-tests

jerop · 2022-05-11T00:01:13Z

/test pull-tekton-pipeline-alpha-integration-tests

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 6, 2022

tekton-robot requested review from dibyom and pritidesai May 6, 2022 20:36

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 6, 2022

jerop force-pushed the tep-0090-validate-params branch from 03c3c50 to cd72aed Compare May 6, 2022 20:43

jerop marked this pull request as draft May 6, 2022 20:49

jerop force-pushed the tep-0090-validate-params branch 2 times, most recently from c7738d9 to f0d1734 Compare May 6, 2022 21:08

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 6, 2022

jerop marked this pull request as ready for review May 6, 2022 21:08

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 6, 2022

lbernick reviewed May 9, 2022

View reviewed changes

jerop force-pushed the tep-0090-validate-params branch from f0d1734 to abc96f5 Compare May 9, 2022 14:00

jerop force-pushed the tep-0090-validate-params branch from abc96f5 to 982324c Compare May 9, 2022 15:06

jerop force-pushed the tep-0090-validate-params branch from 982324c to f1a3998 Compare May 9, 2022 19:02

dibyom approved these changes May 9, 2022

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 9, 2022

tekton-robot assigned abayer May 10, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 10, 2022

jerop marked this pull request as draft May 10, 2022 14:26

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 10, 2022

jerop marked this pull request as ready for review May 10, 2022 14:26

tekton-robot assigned dibyom May 10, 2022

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 10, 2022

tekton-robot merged commit 827e164 into tektoncd:main May 11, 2022

jerop added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 6, 2022

jerop deleted the tep-0090-validate-params branch June 11, 2022 01:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TEP-0090: Include `Matrix` in Validation of `Parameters` in `TaskRun` Reconciler #4844

TEP-0090: Include `Matrix` in Validation of `Parameters` in `TaskRun` Reconciler #4844

jerop commented May 6, 2022 •

edited

Loading

lbernick May 9, 2022

jerop May 9, 2022

lbernick May 9, 2022

dibyom May 9, 2022

jerop May 9, 2022

jerop May 9, 2022

lbernick May 10, 2022

jerop commented May 9, 2022

jerop commented May 9, 2022

jerop commented May 9, 2022

tekton-robot commented May 9, 2022

jerop commented May 9, 2022

tekton-robot commented May 9, 2022

tekton-robot commented May 9, 2022

dibyom commented May 9, 2022

jerop commented May 9, 2022

abayer commented May 10, 2022

jerop commented May 10, 2022

dibyom commented May 10, 2022

jerop commented May 10, 2022

jerop commented May 10, 2022

jerop commented May 10, 2022

jerop commented May 10, 2022

jerop commented May 11, 2022

TEP-0090: Include Matrix in Validation of Parameters in TaskRun Reconciler #4844

TEP-0090: Include Matrix in Validation of Parameters in TaskRun Reconciler #4844

Conversation

jerop commented May 6, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

lbernick May 9, 2022

Choose a reason for hiding this comment

jerop May 9, 2022

Choose a reason for hiding this comment

lbernick May 9, 2022

Choose a reason for hiding this comment

dibyom May 9, 2022

Choose a reason for hiding this comment

jerop May 9, 2022

Choose a reason for hiding this comment

jerop May 9, 2022

Choose a reason for hiding this comment

lbernick May 10, 2022

Choose a reason for hiding this comment

jerop commented May 9, 2022

jerop commented May 9, 2022

jerop commented May 9, 2022

tekton-robot commented May 9, 2022

jerop commented May 9, 2022

tekton-robot commented May 9, 2022

tekton-robot commented May 9, 2022

dibyom commented May 9, 2022

jerop commented May 9, 2022

abayer commented May 10, 2022

jerop commented May 10, 2022

dibyom commented May 10, 2022

jerop commented May 10, 2022

jerop commented May 10, 2022

jerop commented May 10, 2022

jerop commented May 10, 2022

jerop commented May 11, 2022

TEP-0090: Include `Matrix` in Validation of `Parameters` in `TaskRun` Reconciler #4844

TEP-0090: Include `Matrix` in Validation of `Parameters` in `TaskRun` Reconciler #4844

jerop commented May 6, 2022 •

edited

Loading