[TEP-0137] Restructure customrun event controller #6889
Conversation
tekton-robot
added
release-note-none
tekton-robot
added
the
size/XXL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| // IsSuccessful returns true if the TaskRun's status indicates that it has succeeded. | ||
| func (tr *PipelineRun) IsSuccessful() bool { | ||
| return tr != nil && tr.Status.GetCondition(apis.ConditionSucceeded).IsTrue() | ||
| } | ||
|
|
There was a problem hiding this comment.
This is needed so that PipelineRun may implement the RunObject interface
| } | ||
|
|
||
| // RunObject is implemented by Run and CustomRun | ||
| type RunObjectWithRetries interface { | ||
| RunObject | ||
|
|
||
| GetRetryCount() int |
There was a problem hiding this comment.
PipelineRuns do not have retries, so splitting GetRetryCount out to a separate interface
|
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
Coverage for this module is provided indirectly by the tests in runtimeobject_test and in the customrun folder.
I could add some tests here, but I'm not convinced they would add too much value.
|
The following is the coverage report on the affected files.
|
|
/retest |
|
/test check-pr-has-kind-label |
|
@afrittoli: The specified target(s) for
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. |
afrittoli
added
kind/tep
afrittoli
force-pushed
the
tep0137_tektonv1_move
branch
from
1e913d2 to
184309c
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
afrittoli
force-pushed
the
tep0137_tektonv1_move
branch
from
184309c to
65b0eee
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
tekton-robot
added
the
needs-rebase
afrittoli
force-pushed
the
tep0137_tektonv1_move
branch
2 times, most recently
from
9a851e0 to
3be4da5
Compare
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.
|
tekton-robot
added
the
needs-rebase
The events controllers for different resources (CustomRun, TaskRun and PipelineRun) will be almost identical, with only the resource type being different. This commit refactors the CustomRun events controller to factor up as much as possible of the reconciler, controller and test logic so that we can reuse it in the upcoming commits for the other resources. I've done a slight change of strategy in the unit test structure compared to what we do for the core controller tests. A set of tests verifies as much as possible of the shared functions, by mocking the event functionality away. These tests are independent of the specific target format of the events. Most of the functionality of the ReconcileKind functions is handled in reconciler tests, which do not need a controller object or a config map watcher to run, which reduces the complexity of these tests without sacrifying coverage. Finally, a smaller set of tests covers the controller -> reconciler logic, so verify that our controller works well when invoking the ReconcileKind indirectly through the generated package. Signed-off-by: Andrea Frittoli <[email protected]>
afrittoli
force-pushed
the
tep0137_tektonv1_move
branch
from
d4b181c to
b9ac5d8
Compare
tekton-robot
removed
the
needs-rebase
| @@ -23,7 +23,7 @@ import ( | |||
| "knative.dev/pkg/apis" | |||
| ) | |||
|
|
|||
| // RunObject is implemented by CustomRun and Run | |||
| // RunObject is implemented by Run, CustomRun, TaskRun and PipelineRun | |||
There was a problem hiding this comment.
RunObject was introduced for remote resolution but it's not used anymore anywhere.
It is still very useful for the events controller though, except for the GetRetryCount, which I moved to a different interface.
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
tekton-robot
added
the
needs-rebase
|
@afrittoli: PR needs rebase. 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. |
|
@afrittoli do you think we can get this in for this milestone? Or should I move it over to 0.57? |
|
@afrittoli needs a rebase 🙏🏼 |
|
This PR has not made any progress since Pipelines v0.51 when it was initiated introduced. Clearing it from the milestone. Please add back as appropriate. Thanks! |
Dependencies:
Once dependencies are merged, the PR will contain 1 commit only
Changes
[TEP-0137] Restructure customrun event controller
The events controllers for different resources (CustomRun, TaskRun
and PipelineRun) will be almost identical, with only the resource
type being different.
This commit refactors the CustomRun events controller to factor up
as much as possible of the reconciler, controller and test logic
so that we can reuse it in the upcoming commits for the other
resources.
I've done a slight change of strategy in the unit test structure
compared to what we do for the core controller tests.
A set of tests verifies as much as possible of the shared
functions, by mocking the event functionality away. These tests
are independent of the specific target format of the events.
Most of the functionality of the ReconcileKind functions is
handled in reconciler tests, which do not need a controller object
or a config map watcher to run, which reduces the complexity of
these tests without sacrifying coverage.
Finally, a smaller set of tests covers the controller -> reconciler
logic, so verify that our controller works well when invoking
the ReconcileKind indirectly through the generated package.
Signed-off-by: Andrea Frittoli [email protected]
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
/kind misc