-
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
Validate pipelineTask params usage only when explicit declaration is required #6710
Validate pipelineTask params usage only when explicit declaration is required #6710
Conversation
/assign @lbernick |
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
fda063a
to
eb75f96
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
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.
I wouldn't consider this a cleanup PR-- I think it's a bug fix and should have a release note
/kind bug |
eb75f96
to
1dfbadc
Compare
/remove-kind cleanup |
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: lbernick 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 |
This release note might be hard for users to understand: it's not clear what validation is being done, or what "explicit declaration required" might mean. Can you reword to be a bit more concrete? e.g. "Adds validation that parameters used in inline task specs within pipelines are declared by the pipeline"? |
also there is a commit "Revert "[TEP-0091] use VerificationResult in verify"", can we squash the commits to remove it? |
Prior to this, we only had success test cases for propagated params. This led to some hidden validation skips. This PR adds failure test to validate proper usage of pipeline tasks with propagated parameters.
1dfbadc
to
0910cbf
Compare
Fixed! Thanks |
The following is the coverage report on the affected files.
|
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
The following is the coverage report on the affected files.
|
/retest |
Changes
Prior to this, we validated pipeline task params when validating the complete pipeline spec.
This was ok since everything was hidden behind a config flag which was passed down.
However, with the removal of that flag in Issue #6647,
that functionally needs to be tested and fixed because the test coverage was missing for the failure cases.
This PR adds a fix and a failure test
to validate proper usage of pipeline tasks with propagated parameters as described in TEP.
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