Return error if there are multiple same name params #2817
Conversation
|
Hi @NavidZ. Thanks for your PR. I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
2 similar comments
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
|
/kind bug |
|
/assign bobcatfish |
|
/ok-to-test |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
c45eb42 to
fd7ae22
Compare
|
The following is the coverage report on the affected files.
|
| }, { | ||
| Name: "baz", Type: ParamTypeString, Default: &ArrayOrString{Type: ParamTypeString, StringVal: "astring"}, | ||
| }}, | ||
| tasks: []PipelineTask{}, |
There was a problem hiding this comment.
can we add a single valid task such that this test confirms that the pipeline validation does not fail because of no tasks but fails because of duplicate params?
There was a problem hiding this comment.
The empty task list is valid though at least in the current state. So there should be no difference between an empty list and one valid task. Considering that, if you still prefer to have a single valid task here let me know and I'll add it.
There was a problem hiding this comment.
yes please, so that it isolates the tests being done 🙏
There was a problem hiding this comment.
thanks for adding more tests, it now has some coverage for the error being added.
Before:
Now:
| @@ -341,6 +341,9 @@ func validatePipelineParameterVariables(tasks []PipelineTask, params []ParamSpec | |||
| } | |||
| } | |||
|
|
|||
| if _, ok := parameterNames[p.Name]; ok { | |||
There was a problem hiding this comment.
is it possible to add the same condition for arrayParameterNames?
Also, we are still allowing params of different types having same p.Name, i.e. baz of type string and one more param baz of type array. Since we are updating release notes, can we add the check and not permit duplicate param names with different types?
There was a problem hiding this comment.
This logic catches that issue too as far as I can see since this line does execute for all params regardless of their type. I added more tests to show the effect of it and the fact that it works in that case as well. Let me know if I understood what you said correctly or if you wanted me to do something else.
There was a problem hiding this comment.
yeah I see, parameterNames holds all parameters whereas arrayParameterNames is a subset of parameterNames and only holds array params. Sorry for confusion 🙏
fd7ae22 to
c2c9a5b
Compare
Right now pipeline accept multiple parameters with the same name. This sometime causes the pipeline to not finish. This change adds a simple check and returns error when there are multiple parameters with the same name in the spec.params.
c2c9a5b to
eca7ef4
Compare
|
The following is the coverage report on the affected files.
|
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbwsg 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 |
Changes
Right now pipeline accepts multiple parameters with
the same name. This sometime causes the pipeline to
not finish.
This change adds a simple check and returns error
when there are multiple parameters with the same
name in the spec.params.
Fixes #2016
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe release Task to build and release this image.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes