TEP-0090: Include Matrix in Validation of Parameters in TaskRun Reconciler
#4844
Conversation
tekton-robot
added
the
release-note
tekton-robot
added
the
size/M
jerop
force-pushed
the
tep-0090-validate-params
branch
from
03c3c50
to
cd72aed
Compare
jerop
force-pushed
the
tep-0090-validate-params
branch
2 times, most recently
from
c7738d9
to
f0d1734
Compare
tekton-robot
added
the
do-not-merge/work-in-progress
tekton-robot
removed
the
do-not-merge/work-in-progress
| @@ -141,12 +141,26 @@ func wrongTypeParamsNames(params []v1beta1.Param, neededParamsTypes map[string]v | |||
| wrongTypeParamNames = append(wrongTypeParamNames, param.Name) | |||
| } | |||
| } | |||
| for _, param := range matrix { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Rather than pass in matrix params separately, any reason to not just include them in the existing slice of params to validate?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
jerop
force-pushed
the
tep-0090-validate-params
branch
from
f0d1734
to
abc96f5
Compare
|
/retest |
jerop
force-pushed
the
tep-0090-validate-params
branch
from
abc96f5
to
982324c
Compare
|
/retest |
|
/test pull-tekton-pipeline-go-coverage |
|
The following is the coverage report on the affected files.
|
… 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
force-pushed
the
tep-0090-validate-params
branch
from
982324c
to
f1a3998
Compare
|
/test pull-tekton-pipeline-go-coverage |
|
The following is the coverage report on the affected files.
|
|
[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:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
|
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! |
|
/lgtm |
|
/hold |
tekton-robot
added
the
do-not-merge/hold
|
/lgtm |
|
/hold cancel |
tekton-robot
removed
the
do-not-merge/hold
|
/test pull-pipeline-kind-k8s-v1-21-e2e |
|
/test pull-tekton-pipeline-alpha-integration-tests |
2 similar comments
|
/test pull-tekton-pipeline-alpha-integration-tests |
|
/test pull-tekton-pipeline-alpha-integration-tests |
Changes
The
TaskRunreconciler validates that the neededParametersare provided,no extra
Parametersare provide and the types ofParametersare matching.Prior to this change, the reconciler did the above validation considering the
paramsfield in thePipelineTaskonly. In this change, we includematrixfield into the validation routine described above. This includes validating
that
Parametersin theMatrixare substitutingParametersof typeStringin the underlying
Task.Related:
Matrix-Parameters#4704ParametersinTaskRunReconciler #4841Submitter Checklist
As the author of this PR, please check off the items in this checklist:
Release Notes