-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor TaskParam and PipelineParam. #1037
Conversation
Make a new ParamSpec type and place it in its own file along with the already existing Param type. ParamSpec replaces TaskParam and PipelineParam. TaskParam and PipelineParam are identical types, and with future features including array/string parameters, consolidating parameter-related code in param_types.go makes sense for better code-reusability and logical structure. Additionally, I think "ParamSpec" is a better name; TaskParam and PipelineParam currently represent a name and description (with potentially more information in the future such as whether the parameter is an array or string), while the Param type represents the parameter value itself. Previously, it was not as intuitive that Param contains the actual parameter value, not TaskParam/PipelineParam.
For context, I'm currently implementing a way for arrays to be accepted as parameters (#207), and I believe these changes will help improve code reusability for that feature. Either way, though, I still think that this is a good organizational change; almost everyone I have interacted with while trying to implement the feature had trouble keeping track of the difference between |
/test pull-tekton-pipeline-integration-tests |
1 similar comment
/test pull-tekton-pipeline-integration-tests |
Thanks for the quick, well contained clean up @EliZucker ! If that test failure is a flake ( 🤞 ) then: /lgtm |
In response to this:
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. |
v interesting! 🤔 seems consistent anyway, looks like a real problem! |
/test pull-tekton-pipeline-integration-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: EliZucker, 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:
Approvers can indicate their approval by writing |
/retest |
Changes
Taken directly from commit message:
Make a new
ParamSpec
type and place it in its own file along with thealready existing
Param
type.ParamSpec
replacesTaskParam
andPipelineParam
.TaskParam
andPipelineParam
are identical types, and with futurefeatures including array/string parameters, consolidating
parameter-related code in
param_types.go
makes sense for bettercode-reusability and logical structure.
Additionally, I think "ParamSpec" is a better name;
TaskParam
andPipelineParam
currently represent a name and description (withpotentially more information in the future such as whether the parameter
is an array or string), while the
Param
type represents the parametervalue itself. Previously, it was not as intuitive that
Param
containsthe actual parameter value, not
TaskParam
orPipelineParam
.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.
Release Notes
N/A