Remove duplicate code for validating params types and defaults

[chuangw6]

Changes

Prior to this commit, PipelineSpec's ParamSpec type validation
was re-implemented in pipeline_validation.go, which actually has
been implemented in task_validation.go for the ParamSpec's type
validation.

So in this commit, we replace the duplicate code with existing function.

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

NONE

[chuangw6]

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

[lbernick]

Thanks Chuang! I think it could make sense to move parameter validation out of task_validation.go and pipeline_validation.go into a new param_validation.go file since a lot of the param code seems to be duplicated between pipeline and task validation.

[chuangw6]

Thanks Chuang! I think it could make sense to move parameter validation out of task_validation.go and pipeline_validation.go into a new param_validation.go file since a lot of the param code seems to be duplicated between pipeline and task validation.

Hi @lbernick ,
Great point!! I totally agree. I find that duplicate code regarding param validation exists not only here in validation webhook but also in the reconciler level i.e. those two are doing same thing as the code here. But it's a bit tricky for the reconciler level since they are living in different packages i.e. taskrun, pipelinerun.

Would you recommend we also refactor reconciler level to extract a dedicated file? If so, I am happy to do that after finishing the refactor of this validation webhook level.

[chuangw6]

Thanks Chuang! I think it could make sense to move parameter validation out of task_validation.go and pipeline_validation.go into a new param_validation.go file since a lot of the param code seems to be duplicated between pipeline and task validation.

Just thinking that moving param validation related functions to a new file will be a big change for other existing PRs like #4867 #4861 since they are modifying the param validation functions in the original file.
How about we move them to the new file after they get merged? @lbernick

[chuangw6]

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

[lbernick]

Great point!! I totally agree. I find that duplicate code regarding param validation exists not only here in validation webhook but also in the reconciler level i.e. those two are doing same thing as the code here. But it's a bit tricky for the reconciler level since they are living in different packages i.e. taskrun, pipelinerun.

Would you recommend we also refactor reconciler level to extract a dedicated file? If so, I am happy to do that after finishing the refactor of this validation webhook level.

That could definitely make sense! We have something similar for workspaces I think. Agree that it should be a separate PR.

Just thinking that moving param validation related functions to a new file will be a big change for other existing PRs like #4867 #4861 since they are modifying the param validation functions in the original file. How about we move them to the new file after they get merged?

Up to you! I think if you are going to the trouble of making a cleanup PR it's worth putting these functions in the place that makes the most sense (right now this PR is having the pipeline validation use a function defined in task validation which feels a bit strange to me). Rebasing and resolving merge conflicts is sort of a fact of life :) but if you'd prefer to wait that's OK too.

[ywluogg]

/assign ywluogg

[ywluogg]

Up to you! I think if you are going to the trouble of making a cleanup PR it's worth putting these functions in the place that makes the most sense (right now this PR is having the pipeline validation use a function defined in task validation which feels a bit strange to me). Rebasing and resolving merge conflicts is sort of a fact of life :) but if you'd prefer to wait that's OK too.

@chuangw6 maybe change the PR naming to be specifically to "remove duplicate code for validating params types and defaults" to narrow down the cleanup scope. You can create a separate PR for workspaces

[ywluogg]

/lgtm

[tekton-robot]

@ywluogg: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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]

@chuangw6 maybe change the PR naming to be specifically to "remove duplicate code for validating params types and defaults" to narrow down the cleanup scope. You can create a separate PR for workspaces

great point! Done!

[ywluogg]

/approve

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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]

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

[vdemeester]

/lgtm

[afrittoli]

/kind misc