-
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
[TEP100] Remove Taskruns
and Runs
Fields for PipelineRunStatus
#6099
[TEP100] Remove Taskruns
and Runs
Fields for PipelineRunStatus
#6099
Conversation
Skipping CI for Draft Pull Request. |
The following is the coverage report on the affected files.
|
Taskruns
and Runs
Fields for PipelineRunStatus
Taskruns
and Runs
Fields for PipelineRunStatusTaskruns
and Runs
Fields for PipelineRunStatus
2f738c7
to
7a410ca
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go
Outdated
Show resolved
Hide resolved
7a410ca
to
57a1f32
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.
@JeromeJu please update the PipelineRun
status documentation as well - https://github.com/tektoncd/pipeline/blob/main/docs/pipelineruns.md#pipelinerun-status
57a1f32
to
5b4ea9b
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
Thank you @JeromeJu for all the hard work on this 🙏 Since this is one of the last PRs and mainly related to the feature which involves deleting existing code, I request to maintain the existing go coverage, thanks! |
The changes looks good to me, it will be great if you can improve the code coverage otherwise happy to merge, thanks! |
This commit removes `status.taskruns` and `status.runs` for PipelineRunStatus since they have been deprecated. The corresponding usage in the test cases are replaced with `childReferences`.
5b4ea9b
to
1d27ad7
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
I was wondering if there are any approaches suggested here or is this inevitable? Currently I have tried to add ChildReferences cases to pipelinerun reconciler unit tests as replacement for From the coverage report, there are some other uncovered cases could be added but I don't think they would be suitable to fit in this PR since they are not relevant to
|
thank you @JeromeJu 👍 The test coverage is certainly not a blocker here. Happy to merge and you can follow up improving coverage in a separate PR. Here are some pointers to add more coverage: The coverage before and after of pkg/apis/pipeline/v1beta1/pipelinerun_types.go: The coverage before and after of pkg/reconciler/pipelinerun/pipelinerun.go: |
Please confirm the beta integration test failure is not a flake and I can merge this, thank you! 🙏 |
/retest |
Thanks @pritidesai for the pointers! I created #6159 and will follow up in a separate PR! Also confirmed with @XinruZhang offline that the beta integration is an known flake where the network glitches for CI. |
thank you @JeromeJu 🙏 /lgtm |
Changes
This commit removes
status.taskruns
andstatus.runs
forPipelineRunStatus since they have been deprecated. The corresponding usage in
the test cases are replaced with
childReferences
.Closes #4954
fixes #6090
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