validate matrix for duplicate params #6308
Conversation
tekton-robot
added
release-note
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
also let's add a docs update - https://github.com/tektoncd/pipeline/blob/main/docs/matrix.md#parameters
pritidesai
force-pushed
the
validate-matrix-for-duplicate-params
branch
from
7a05011
to
9001a58
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
pritidesai
force-pushed
the
validate-matrix-for-duplicate-params
branch
from
9001a58
to
eed059d
Compare
In case of task/pipeline params, the validation we have in place catches duplicate params i.e. parameters defined multiple times with the same name results in a validation error. The same validation was missing from matrix.params and matrix.Include. matrix.params also takes a list of array parameters where a user could define the same parameter twice by mistake. Signed-off-by: pritidesai <pdesai@us.ibm.com>
pritidesai
force-pushed
the
validate-matrix-for-duplicate-params
branch
from
eed059d
to
56f0ed8
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
thank you @jerop for the review, overall I have rephrased the validation error from: to: |
| @@ -275,7 +275,7 @@ func (pt *PipelineTask) validateMatrix(ctx context.Context) (errs *apis.FieldErr | |||
| errs = errs.Also(pt.Matrix.validateCombinationsCount(ctx)) | |||
| } | |||
| errs = errs.Also(pt.Matrix.validateParameterInOneOfMatrixOrParams(pt.Params)) | |||
There was a problem hiding this comment.
wondering about the overlap between this validation for uniqueness and the one in validateParameterInOneOfMatrixOrParams -- maybe this check should be for all params in a pipelinetask (whether it's in params or matrix.params or matrix.include.params) -- wdyt @pritidesai?
There was a problem hiding this comment.
summary: can we remove validateParameterInOneOfMatrixOrParams?
There was a problem hiding this comment.
I suggest not removing validateParameterInOneOfMatrixOrParams.
validateParameterInOneOfMatrixOrParams checks the params against the pipelineTask.params and validates that a pipelineTask.matrix.Params.foobar is not defined in pipelineTask.params. This validation serves its purpose of comparing params in two separate sections which is still necessary. It is necessary because the error being reported is very clear that a param can either exist in a pipelineTask.params or pipelineTask.matrix.params. What it does not catch is duplicates within pipelineTask.matrix.Params since a set is created based on pipelineTask.matrix.Params. The set will automatically make sure to have just a single occurrence of any parameter.
The duplicate check validateDuplicateParameters each per section, helps identify where exactly the duplicates are and validateParameterInOneOfMatrixOrParams compares two sections with a clear error message.
So, to summarize, I would keep these checks as is. Let me know if you think different. I am happy to apply more simplification.
Now, what it reminds me of, we do not have a test for checking if pipelineTask.matrix.include.params does not exist in pipelineTask.params.
so instead of only iterating over m.params, collect params defined in matrix.include.params:
func (m *Matrix) validateParameterInOneOfMatrixOrParams(params []Param) (errs *apis.FieldError) {
matrixParameterNames := sets.NewString()
if m != nil {
for _, param := range m.Params {
matrixParameterNames.Insert(param.Name)
}
for _, param := range m.Include.Params {
matrixParameterNames.Insert(param.Name)
}
}
for _, param := range params {
if matrixParameterNames.Has(param.Name) {
errs = errs.Also(apis.ErrMultipleOneOf("matrix["+param.Name+"]", "params["+param.Name+"]"))
}
}
return errs
}
thoughts?
There was a problem hiding this comment.
sounds good to keep pt.validateParameterInOneOfMatrixOrParams and m.validateDuplicateParameters separate
and agreed with checking for duplicates between pt.params and m.include.params -- the code sample you have looks great to me
cc @EmmaMunley
There was a problem hiding this comment.
Thank you Priti! Opened a PR for this here to include in next milestone release: #6349
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
|
/retest |
1 similar comment
|
/retest |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
Changes
In case of task/pipeline params, the validation we have in place catches duplicate params i.e. parameters defined multiple times with the same name results in a validation error. The same validation was missing from
matrix.paramsandmatrix.Include.params. This PR is implementing such validation.matrix.paramsandmatrix.include.params.also specifies a list of array parameters where a user could define the same parameter twice by mistake./kind bug
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes