[TEP074] Remove PipelineResourceResultType of PipelineResourceResult Struct #6198
Conversation
|
Skipping CI for Draft Pull Request. |
tekton-robot
added
do-not-merge/work-in-progress
|
The following is the coverage report on the affected files.
|
JeromeJu
force-pushed
the
remove-resource-result-type
branch
from
099f0a0
to
382151a
Compare
|
/kind misc |
tekton-robot
added
the
kind/misc
tekton-robot
added
size/XL
|
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.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
third_party/github.com/hashicorp/go-secure-stdlib/parseutil/parseutil.go
Fixed
Show fixed
Hide fixed
third_party/github.com/hashicorp/go-secure-stdlib/parseutil/parseutil.go
Fixed
Show fixed
Hide fixed
JeromeJu
force-pushed
the
remove-resource-result-type
branch
from
652ca8f
to
5e9a86a
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
JeromeJu
force-pushed
the
remove-resource-result-type
branch
from
5e9a86a
to
29d4eda
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
JeromeJu
changed the title
tekton-robot
removed
the
do-not-merge/work-in-progress
JeromeJu
changed the title
|
/retest |
JeromeJu
force-pushed
the
remove-resource-result-type
branch
from
29d4eda
to
5f5188d
Compare
|
Thanks for the reviews and comments. |
This commit removes the PipelineResourceResultType of PipelineResourceResult Struct given now the PipelineResources have been removed.
JeromeJu
force-pushed
the
remove-resource-result-type
branch
from
903a4eb
to
aa6cfe8
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
JeromeJu
requested review from
pritidesai,
chitrangpatel,
Yongxuanzhang and
lbernick
and removed request for
imjasonh,
bobcatfish,
dlorenc,
wlynch,
pritidesai,
chitrangpatel and
Yongxuanzhang
| @@ -290,8 +290,6 @@ func filterResultsAndResources(results []v1beta1.PipelineResourceResult, specRes | |||
| case v1beta1.InternalTektonResultType: | |||
| // Internal messages are ignored because they're not used as external result | |||
| continue | |||
| case v1beta1.PipelineResourceResultType: | |||
There was a problem hiding this comment.
This function can be simplified now. The first return value is any TaskRunResults, the second value is any PipelineResourceResults of type PipelineResourceResultType or UnknownResultType, and the third return value is all PipelineResourceResults that aren't InternalTektonResultTypes.
I'm not sure UnknownResourceTypes are actually used anywhere. It might be simpler to just have one function that filters a list of PipelineResourceResults and returns any that are of type TaskRunResultType, and another function that turns those results into TaskRunResults.
Please also add a docstring.
There was a problem hiding this comment.
On second thought I think this can happen separately.
| @@ -290,8 +290,6 @@ func filterResultsAndResources(results []v1beta1.PipelineResourceResult, specRes | |||
| case v1beta1.InternalTektonResultType: | |||
| // Internal messages are ignored because they're not used as external result | |||
| continue | |||
| case v1beta1.PipelineResourceResultType: | |||
There was a problem hiding this comment.
On second thought I think this can happen separately.
|
[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 |
tekton-robot
added
approved
|
I do appreciate the time PLUS guidance on this @lbernick 😃 And to put the above pointers in a summary, on the next step:
|
yeah that sgtm, I'd focus on the changes needed for removing pipelineresources-- I started poking around with a readability refactor to status.go |
|
/lgtm |
|
/test pull-tekton-pipeline-alpha-integration-tests |
Changes
PipelineResourceResult Struct is a blocker for the cleanup of
PipelineResource.PipelineResourceResult Struct was first introduced just to write digest as results for image resources but over the time,
generalized to take in more results. But it really is just an abstraction of
TaskrunResult,ResourceResultandInternalTektonResult. And since now the pipelineResources is going to be removed, it is no longer going to be living in the apis as inresource_typeswe would need to move it off the apis and rename it.pipelineResourceResultTypesince pipelineResources aregoing to be removed
Part of: #6197
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