[TEP-0091] return VerificationResult from GetPipeline #6736
Conversation
|
Skipping CI for Draft Pull Request. |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
| @@ -423,6 +423,12 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get | |||
| } | |||
| } | |||
|
|
|||
| if pipelineMeta.VerificationResult != nil && pipelineMeta.VerificationResult.VerificationResultType == trustedresources.VerificationError { | |||
There was a problem hiding this comment.
should we remove the case errors.Is(err, trustedresources.ErrResourceVerificationFailed)? Also, is there a test for marking the PipelineRun failed when GetPipelineData returns a verificationResult w/ type VerificationError?
There was a problem hiding this comment.
This is to keep the pipelinerun.go behave like before, we have an error check above
case errors.Is(err, trustedresources.ErrResourceVerificationFailed):
message := fmt.Sprintf("PipelineRun %s/%s referred pipeline failed signature verification", pr.Namespace, pr.Name)
pr.Status.MarkFailed(ReasonResourceVerificationFailed, message)
return controller.NewPermanentError(err)
I just realized that I should remove that error check since we now rely on the VerificationResult.
For the test, yes, we have reconciler tests here:
This can make sure if GetPipelineData returns a verificationResult w/ type VerificationError, the pipelinerun is marked as failed.
I will openother PRs to update taskrun and pipelinerun code to check the VerificationResult and update conditions.
4a9a205 to
2ee1ac7
Compare
|
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 |
|
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
Thank you @Yongxuanzhang !
Out of scope of this PR:
I see a pattern existing in some functions i.e. readRuntimeObjectAsPipeline, resolvePipeline and GetPipeline that all need return two of or all of the three results Pipeline, refsource and VerificationResult.
And we are just expanding the number of return values as needed i.e. I added refSource in previous project, and you introduced VerificationResult. This will become hard to manage and understand for others if we want to introduce more meta fields in future.
A potential cleanup is to pass those values through a single struct and we already have a meta struct that we can leverage
Example:
type ResolvedPipeline {
pipeline *v1beta1.Pipeline
resolvedMeta *resolution.ResolvedObjectMeta
}Same for task level
type ResolvedTask {
task *v1beta1.Task
resolvedMeta *resolution.ResolvedObjectMeta
}WDYT?
| expected runtime.Object | ||
| expectedRefSource *v1beta1.RefSource | ||
| expectedVerificationResult *trustedresources.VerificationResult | ||
| }{{ |
There was a problem hiding this comment.
https://google.github.io/styleguide/go/decisions#got-before-want
| expected runtime.Object | |
| expectedRefSource *v1beta1.RefSource | |
| expectedVerificationResult *trustedresources.VerificationResult | |
| }{{ | |
| wantPipeline runtime.Object | |
| wantRefSource *v1beta1.RefSource | |
| wantExpectedVerificationResult *trustedresources.VerificationResult | |
| }{{ |
There was a problem hiding this comment.
I will update this in another PR for all trusted resources tests
I had the same thought, probably open a PR later when I finish all the work |
2ee1ac7 to
68b91aa
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
68b91aa to
979d351
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
979d351 to
9a9ff28
Compare
|
The following is the coverage report on the affected files.
|
This commits updates GetPipeline to return VerificationResult. VerificationResult is returned by VerifyResource in remote resolution. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com
9a9ff28 to
130ffa2
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/lgtm |
Changes
This commits updates GetPipeline to return
VerificationResult. VerificationResult is returned by VerifyResource in remote resolution.
4th PR of #6665
This PR is similar to #6691
/kind feature
Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com
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