-
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 validation of propagated parameters and workspaces #6660
Conversation
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
/retest |
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.
Thanks! It looks really nice! Left minor comments.
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
while I am trying to understand the changes in this PR, does |
I don't think so. That function checks that the defaults provided have the type of the param declared. Since we can do this in the webhook (and it's not currently guarded by the sentinel value), no need to move it into the separate validation function that will be called in the reconciler. No functional changes intended in this PR. |
When using propagated parameters and workspaces, params/workspaces referenced in a Task or Pipeline are not necessarily declared in that Task/Pipeline. We currently skip some validation that checks whether referenced params/workspaces are declared based on sentinel values injected into the validation context. This commit consolidates validation logic that is gated behind this check. This refactoring will make it easier to remove these sentinel values and perform this validation only where it is supposed to happen. The only functional changes expected as a result of this commit are that we now return a multierror if multiple object parameter declarations are missing keys, instead of only returning an error for the first one.
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chitrangpatel, dibyom 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 |
/lgtm |
When using propagated parameters and workspaces, params/workspaces referenced in a Task or Pipeline are not necessarily declared in that Task/Pipeline. We currently skip some validation that checks whether referenced params/workspaces are declared based on sentinel values injected into the validation context.
This commit consolidates validation logic that is gated behind this check. This refactoring will make it easier to remove these sentinel values and perform this validation only where it is supposed to happen. The only functional changes expected as a result of this commit are that we now return a multierror if multiple object parameter declarations are missing keys, instead of only returning an error for the first one.
/kind cleanup
Partially addresses #6647
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes