-
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
Fail taskrun when results validation fails #5198
Conversation
Skipping CI for Draft Pull Request. |
5ba9896
to
2dec008
Compare
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
name: "taskrun results type mismatched", | ||
taskRun: taskRunResultsTypeMismatched, | ||
wantFailedReason: podconvert.ReasonFailedValidation, | ||
expectedError: fmt.Errorf("1 error occurred:\n\t* missmatched Types for these results, map[aResult:[array]]\n\n"), |
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.
This is a confusing error message because it's not clear which result (taskrun result or result declared in task spec) is an array and which is not, and what the type of the result that is not an array is. Can it be improved?
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.
The current code logic will only collect the wrong results/params names and log them, and same for other parts. You're 100% correct that this can be improved! I will think about it and maybe address it in a new PR? If that is ok I will open an issue to track
pipeline/pkg/reconciler/pipelinerun/resources/validate_params.go
Lines 82 to 90 in 8a7b0cf
// ValidateObjectParamRequiredKeys validates that the required keys of all the object parameters expected by the Pipeline are provided by the PipelineRun. | |
func ValidateObjectParamRequiredKeys(pipelineParameters []v1beta1.ParamSpec, pipelineRunParameters []v1beta1.Param) error { | |
missings := taskrun.MissingKeysObjectParamNames(pipelineParameters, pipelineRunParameters) | |
if len(missings) != 0 { | |
return fmt.Errorf("PipelineRun missing object keys for parameters: %v", missings) | |
} | |
return nil | |
} |
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.
Sure, you can definitely change in a separate PR-- it's a small enough change I'll leave it up to you whether to create an issue or just open a follow-up PR
}, | ||
}}, | ||
} | ||
for _, tc := range []struct { |
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.
could you add a test case for success if there isn't one already in taskrun_test.go?
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.
sure! just added one test for success
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.
It looks like you might have accidentally just copied the failure test case?
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.
oh yes, sorry about that. >_< I only updated the yaml name. Should be correct now!
2dec008
to
0994c48
Compare
The following is the coverage report on the affected files.
|
0994c48
to
7712256
Compare
The following is the coverage report on the affected files.
|
/retest |
1 similar comment
/retest |
/test |
@Yongxuanzhang: The
The following commands are available to trigger optional jobs:
Use 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. |
/test |
@Yongxuanzhang: The
The following commands are available to trigger optional jobs:
Use 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. |
/retest |
name: "taskrun results type mismatched", | ||
taskRun: taskRunResultsTypeMismatched, | ||
}, { | ||
name: "taskrun results object miss key", | ||
taskRun: taskRunResultsObjectMissKey, |
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.
could you please rename these test cases and variables so it's clear what's being tested?
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.
sure!
This commit adds the ReasonFailedValidation into taskrun status and fail the taskrun when results validation fails. Before this commit users can only see error messages in logs but the taskrun are completed like no errors happen.
7712256
to
bfeec8f
Compare
The following is the coverage report on the affected files.
|
/retest |
[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 |
/lgtm |
Changes
This commit adds the ReasonFailedValidation into taskrun status and fail
the taskrun when results validation fails. Before this commit users can
only see error messages in logs but the taskrun are completed like no
errors happen.
/kind bug
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes