Add validation for duplicated param names in TaskSpec #4806

chitrangpatel · 2022-04-27T19:13:55Z

Prior to this, when duplicated names for parameters were assigned in
taskSpec, the param value would be overwritten by the last value.

Changes

To avoid this, a validation check was added to ensure that no param name
is duplicated in TaskSpec. This PR address issue #4798.

Submitter Checklist

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

Add validation for duplicated param names in TaskSpec.

tekton-robot · 2022-04-27T19:14:10Z

Hi @chitrangpatel. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

chuangw6 · 2022-04-27T19:37:31Z

/ok-to-test

tekton-robot · 2022-04-27T19:40:00Z

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/apis/pipeline/v1beta1/task_validation.go	97.1%	97.2%	0.1

tekton-robot · 2022-04-28T12:33:14Z

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/apis/pipeline/v1beta1/task_validation.go	97.1%	97.2%	0.1

abayer · 2022-04-28T15:58:06Z

/retest

tekton-robot · 2022-04-29T15:07:06Z

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/apis/pipeline/v1beta1/task_validation.go	97.1%	97.2%	0.1

dibyom · 2022-04-29T18:27:24Z

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

tekton-robot · 2022-04-29T20:09:35Z

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/apis/pipeline/v1beta1/task_validation.go	97.1%	97.2%	0.1

chitrangpatel · 2022-05-03T18:25:36Z

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

vdemeester · 2022-05-05T08:49:25Z

pkg/apis/pipeline/v1beta1/task_validation.go

+	errs = errs.Also(validateParameterNames(ts.Params).ViaField("params"))
 	errs = errs.Also(ValidateParameterTypes(ts.Params).ViaField("params"))


nit: we could have a ValidateParamSpecs function that does both of those at the same time. Probably an opmitization at that point of time though.

chitrangpatel · 2022-05-06T15:55:02Z

/retest

chitrangpatel · 2022-05-09T18:01:35Z

/retest

tekton-robot · 2022-05-09T18:04:04Z

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/apis/pipeline/v1beta1/task_validation.go	97.0%	97.1%	0.1

chitrangpatel · 2022-05-11T19:37:17Z

/assign @vdemeester

lbernick

Thanks Chitrang for fixing this issue!

lbernick · 2022-05-17T08:18:01Z

pkg/apis/pipeline/v1beta1/task_validation.go

@@ -244,6 +244,17 @@ func validateStep(ctx context.Context, s Step, names sets.String) (errs *apis.Fi
 	return errs
 }

+// ValidateParameterSpecs validates that there are no duplicate names in the TaskSpec and all the types within a slice of ParamSpecs.
+func validateParameterSpecs(params []ParamSpec) (errs *apis.FieldError) {


I think it might make more sense to add this validation to the function ValidateParameterVariables, and likewise in ValidatePipelineParameterVariables

Agreed! I made the necessary changes.

It doesn't look like the changes were applied to ValidatePipelineParameterVariables?

Also, no need to make any changes to v1alpha1

validatePipelineParameterVariables was already checking for duplicated param names and param types so no changed needed to be made there. It was only missing at the task validation stage.

Yes, now that I moved the function call to validateParamTypes back to where it was, ctx was no longer required by ValidateParameterVariables and so the change to v1alpha1 was also undone.

tekton-robot · 2022-05-17T21:08:11Z

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/apis/pipeline/v1beta1/task_validation.go	97.7%	97.7%	0.0

lbernick · 2022-05-18T08:16:08Z

pkg/apis/pipeline/v1beta1/task_validation.go

-func ValidateParameterVariables(steps []Step, params []ParamSpec) *apis.FieldError {
+func ValidateParameterVariables(ctx context.Context, steps []Step, params []ParamSpec) *apis.FieldError {
+	// Validate Parameter Types
+	errs := ValidateParameterTypes(ctx, params).ViaField("params")


I think the call to this function is OK to stay where it was

Right. That will simplify things since I don't need to pass in the context which is why I also had to make changes to v1alpha1.

Prior to this, when duplicated names for parameters were assigned in `taskSpec`, the param value would be overwritten by the last value. To avoid this, a validation check was added to ensure that no param name is duplicated in TaskSpec.

tekton-robot · 2022-05-18T12:47:57Z

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/apis/pipeline/v1beta1/task_validation.go	97.7%	97.7%	0.0

vdemeester

/lgtm

tekton-robot · 2022-05-20T08:32:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lbernick, vdemeester

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:

~~pkg/apis/OWNERS~~ [lbernick,vdemeester]

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 the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Apr 27, 2022

tekton-robot requested review from bobcatfish and pritidesai April 27, 2022 19:14

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

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 27, 2022

chitrangpatel mentioned this pull request Apr 27, 2022

Missing validation against duplicated parameter names #4798

Closed

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 27, 2022

chitrangpatel force-pushed the Issue-4798-Missing-validation-against-duplicated-parameter-names branch from d22579e to 09a0bc2 Compare April 28, 2022 12:28

chuangw6 mentioned this pull request May 1, 2022

Case-insensitive duplication check in TaskRunSpec's params #4814

Closed

vdemeester reviewed May 5, 2022

View reviewed changes

chitrangpatel force-pushed the Issue-4798-Missing-validation-against-duplicated-parameter-names branch from f09d585 to 09e8c44 Compare May 9, 2022 17:15

chitrangpatel changed the title ~~Issue #4798 missing validation against duplicated parameter names~~ Add validation for duplicated param names in TaskSpec May 9, 2022

chitrangpatel force-pushed the Issue-4798-Missing-validation-against-duplicated-parameter-names branch from 09e8c44 to d4f8711 Compare May 9, 2022 17:39

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 11, 2022

tekton-robot assigned vdemeester May 11, 2022

lbernick reviewed May 17, 2022

View reviewed changes

chitrangpatel force-pushed the Issue-4798-Missing-validation-against-duplicated-parameter-names branch 2 times, most recently from 05aa6fe to b30f933 Compare May 17, 2022 21:05

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 17, 2022

lbernick reviewed May 18, 2022

View reviewed changes

chitrangpatel force-pushed the Issue-4798-Missing-validation-against-duplicated-parameter-names branch from b30f933 to 1d67120 Compare May 18, 2022 12:44

tekton-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 18, 2022

lbernick approved these changes May 18, 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 18, 2022

vdemeester approved these changes May 20, 2022

View reviewed changes

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

tekton-robot merged commit 3a305a0 into tektoncd:main May 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add validation for duplicated param names in TaskSpec #4806

Add validation for duplicated param names in TaskSpec #4806

chitrangpatel commented Apr 27, 2022 •

edited

Loading

tekton-robot commented Apr 27, 2022

chuangw6 commented Apr 27, 2022

tekton-robot commented Apr 27, 2022

tekton-robot commented Apr 28, 2022

abayer commented Apr 28, 2022

tekton-robot commented Apr 29, 2022

dibyom commented Apr 29, 2022

tekton-robot commented Apr 29, 2022

chitrangpatel commented May 3, 2022

vdemeester May 5, 2022

chitrangpatel commented May 6, 2022

chitrangpatel commented May 9, 2022

tekton-robot commented May 9, 2022

chitrangpatel commented May 11, 2022

lbernick left a comment

lbernick May 17, 2022

chitrangpatel May 17, 2022

lbernick May 18, 2022

chitrangpatel May 18, 2022

tekton-robot commented May 17, 2022

lbernick May 18, 2022

chitrangpatel May 18, 2022

tekton-robot commented May 18, 2022

vdemeester left a comment

tekton-robot commented May 20, 2022

		errs = errs.Also(validateParameterNames(ts.Params).ViaField("params"))
		errs = errs.Also(ValidateParameterTypes(ts.Params).ViaField("params"))

Add validation for duplicated param names in TaskSpec #4806

Add validation for duplicated param names in TaskSpec #4806

Conversation

chitrangpatel commented Apr 27, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Apr 27, 2022

chuangw6 commented Apr 27, 2022

tekton-robot commented Apr 27, 2022

tekton-robot commented Apr 28, 2022

abayer commented Apr 28, 2022

tekton-robot commented Apr 29, 2022

dibyom commented Apr 29, 2022

tekton-robot commented Apr 29, 2022

chitrangpatel commented May 3, 2022

vdemeester May 5, 2022

Choose a reason for hiding this comment

chitrangpatel commented May 6, 2022

chitrangpatel commented May 9, 2022

tekton-robot commented May 9, 2022

chitrangpatel commented May 11, 2022

lbernick left a comment

Choose a reason for hiding this comment

lbernick May 17, 2022

Choose a reason for hiding this comment

chitrangpatel May 17, 2022

Choose a reason for hiding this comment

lbernick May 18, 2022

Choose a reason for hiding this comment

chitrangpatel May 18, 2022

Choose a reason for hiding this comment

tekton-robot commented May 17, 2022

lbernick May 18, 2022

Choose a reason for hiding this comment

chitrangpatel May 18, 2022

Choose a reason for hiding this comment

tekton-robot commented May 18, 2022

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented May 20, 2022

chitrangpatel commented Apr 27, 2022 •

edited

Loading