[TEP-0137] Restructure customrun event controller #6889

afrittoli · 2023-06-29T16:54:31Z

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 andrea.frittoli@uk.ibm.com

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

/kind misc

tekton-robot · 2023-06-29T16:54:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from afrittoli after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

afrittoli · 2023-06-29T16:56:21Z

pkg/apis/pipeline/v1/pipelinerun_types.go

+// 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()
+}
+


This is needed so that PipelineRun may implement the RunObject interface

afrittoli · 2023-06-29T16:57:37Z

pkg/apis/pipeline/v1beta1/run_interface.go

+}
+
+// RunObject is implemented by Run and CustomRun
+type RunObjectWithRetries interface {
+	RunObject

 	GetRetryCount() int


PipelineRuns do not have retries, so splitting GetRetryCount out to a separate interface

tekton-robot · 2023-06-29T16:59:56Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/events.go	Do not exist	98.1%
pkg/apis/config/store.go	91.7%	92.6%	0.9
pkg/apis/pipeline/v1/pipelinerun_types.go	89.5%	88.7%	-0.8
pkg/apis/pipeline/v1beta1/pipelinerun_types.go	89.7%	88.9%	-0.8
pkg/reconciler/events/cloudevent/cloud_event_controller.go	84.9%	84.6%	-0.3
pkg/reconciler/notifications/controller.go	Do not exist	25.0%
pkg/reconciler/notifications/runtimeobject.go	Do not exist	100.0%
pkg/reconciler/notifications/testing.go	Do not exist	92.3%
test/controller.go	29.4%	30.1%	0.7

afrittoli · 2023-06-29T17:00:54Z

pkg/reconciler/notifications/controller.go

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.

tekton-robot · 2023-06-29T17:01:12Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/events.go	Do not exist	98.1%
pkg/apis/config/store.go	91.7%	92.6%	0.9
pkg/apis/pipeline/v1/pipelinerun_types.go	89.5%	88.7%	-0.8
pkg/apis/pipeline/v1beta1/pipelinerun_types.go	89.7%	88.9%	-0.8
pkg/reconciler/events/cloudevent/cloud_event_controller.go	84.9%	84.6%	-0.3
pkg/reconciler/notifications/controller.go	Do not exist	25.0%
pkg/reconciler/notifications/runtimeobject.go	Do not exist	100.0%
pkg/reconciler/notifications/testing.go	Do not exist	92.3%
test/controller.go	29.4%	30.1%	0.7

afrittoli · 2023-06-30T09:15:31Z

/retest

afrittoli · 2023-06-30T09:15:57Z

/test check-pr-has-kind-label

tekton-robot · 2023-06-30T09:15:59Z

@afrittoli: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-tekton-pipeline-alpha-integration-tests
/test pull-tekton-pipeline-beta-integration-tests
/test pull-tekton-pipeline-build-tests
/test pull-tekton-pipeline-integration-tests
/test tekton-pipeline-unit-tests

The following commands are available to trigger optional jobs:

/test pull-tekton-pipeline-go-coverage

Use /test all to run all jobs.

In response to this:

/test check-pr-has-kind-label

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.

tekton-robot · 2023-06-30T10:02:32Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/events.go	Do not exist	98.1%
pkg/apis/config/store.go	91.7%	92.6%	0.9
pkg/apis/pipeline/v1/pipelinerun_types.go	89.5%	88.7%	-0.8
pkg/apis/pipeline/v1beta1/pipelinerun_types.go	89.7%	88.9%	-0.8
pkg/reconciler/events/cloudevent/cloud_event_controller.go	84.9%	84.6%	-0.3
pkg/reconciler/notifications/customrun/controller.go	Do not exist	100.0%
pkg/reconciler/notifications/customrun/reconciler.go	Do not exist	87.5%
test/controller.go	29.4%	30.1%	0.7

tekton-robot · 2023-06-30T10:05:40Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/events.go	Do not exist	98.1%
pkg/apis/config/store.go	91.7%	92.6%	0.9
pkg/apis/pipeline/v1/pipelinerun_types.go	89.5%	88.7%	-0.8
pkg/apis/pipeline/v1beta1/pipelinerun_types.go	89.7%	88.9%	-0.8
pkg/reconciler/events/cloudevent/cloud_event_controller.go	84.9%	84.6%	-0.3
pkg/reconciler/notifications/customrun/controller.go	Do not exist	100.0%
pkg/reconciler/notifications/customrun/reconciler.go	Do not exist	87.5%
test/controller.go	29.4%	30.1%	0.7

tekton-robot · 2023-06-30T10:21:59Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/events.go	Do not exist	98.1%
pkg/apis/config/store.go	91.7%	92.6%	0.9
pkg/apis/pipeline/v1/pipelinerun_types.go	89.5%	88.7%	-0.8
pkg/apis/pipeline/v1beta1/pipelinerun_types.go	89.7%	88.9%	-0.8
pkg/reconciler/events/cloudevent/cloud_event_controller.go	84.9%	84.6%	-0.3
pkg/reconciler/notifications/controller.go	Do not exist	25.0%
pkg/reconciler/notifications/customrun/controller.go	Do not exist	100.0%
pkg/reconciler/notifications/customrun/reconciler.go	Do not exist	87.5%
pkg/reconciler/notifications/runtimeobject.go	Do not exist	100.0%
pkg/reconciler/notifications/testing.go	Do not exist	96.2%
test/controller.go	29.4%	30.1%	0.7

tekton-robot · 2023-06-30T10:24:38Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/events.go	Do not exist	98.1%
pkg/apis/config/store.go	91.7%	92.6%	0.9
pkg/apis/pipeline/v1/pipelinerun_types.go	89.5%	88.7%	-0.8
pkg/apis/pipeline/v1beta1/pipelinerun_types.go	89.7%	88.9%	-0.8
pkg/reconciler/events/cloudevent/cloud_event_controller.go	84.9%	84.6%	-0.3
pkg/reconciler/notifications/controller.go	Do not exist	25.0%
pkg/reconciler/notifications/customrun/controller.go	Do not exist	100.0%
pkg/reconciler/notifications/customrun/reconciler.go	Do not exist	87.5%
pkg/reconciler/notifications/runtimeobject.go	Do not exist	100.0%
pkg/reconciler/notifications/testing.go	Do not exist	96.2%
test/controller.go	29.4%	30.1%	0.7

tekton-robot · 2023-07-03T12:39:27Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/pipelinerun_types.go	89.5%	88.7%	-0.8
pkg/apis/pipeline/v1beta1/pipelinerun_types.go	89.7%	88.9%	-0.8
pkg/reconciler/notifications/controller.go	Do not exist	25.0%
pkg/reconciler/notifications/customrun/controller.go	Do not exist	100.0%
pkg/reconciler/notifications/customrun/reconciler.go	Do not exist	87.5%
pkg/reconciler/notifications/runtimeobject.go	Do not exist	100.0%

tekton-robot · 2023-07-03T12:40:55Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/pipelinerun_types.go	89.5%	88.7%	-0.8
pkg/apis/pipeline/v1beta1/pipelinerun_types.go	89.7%	88.9%	-0.8
pkg/reconciler/notifications/controller.go	Do not exist	25.0%
pkg/reconciler/notifications/customrun/controller.go	Do not exist	100.0%
pkg/reconciler/notifications/customrun/reconciler.go	Do not exist	87.5%
pkg/reconciler/notifications/runtimeobject.go	Do not exist	100.0%

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 <andrea.frittoli@uk.ibm.com>

afrittoli · 2023-07-10T12:28:32Z

pkg/apis/pipeline/v1beta1/run_interface.go

@@ -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


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.

tekton-robot · 2023-07-10T12:31:54Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/pipelinerun_types.go	89.5%	88.7%	-0.8
pkg/apis/pipeline/v1beta1/pipelinerun_types.go	89.7%	88.9%	-0.8
pkg/reconciler/notifications/controller.go	Do not exist	25.0%
pkg/reconciler/notifications/customrun/reconciler.go	Do not exist	87.5%
pkg/reconciler/notifications/runtimeobject.go	Do not exist	100.0%

tekton-robot · 2023-07-10T12:32:37Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1/pipelinerun_types.go	89.5%	88.7%	-0.8
pkg/apis/pipeline/v1beta1/pipelinerun_types.go	89.7%	88.9%	-0.8
pkg/reconciler/notifications/controller.go	Do not exist	25.0%
pkg/reconciler/notifications/customrun/reconciler.go	Do not exist	87.5%
pkg/reconciler/notifications/runtimeobject.go	Do not exist	100.0%

tekton-robot · 2023-08-08T20:05:24Z

@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.

chitrangpatel · 2024-01-10T15:27:54Z

@afrittoli do you think we can get this in for this milestone? Or should I move it over to 0.57?

vdemeester · 2024-01-24T12:03:11Z

@afrittoli needs a rebase 🙏🏼

pritidesai · 2024-03-20T16:42:43Z

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!

tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. kind/misc Categorizes issue or PR as a miscellaneuous one. labels Jun 29, 2023

tekton-robot requested review from imjasonh and pritidesai June 29, 2023 16:54

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 29, 2023

afrittoli commented Jun 29, 2023

View reviewed changes

afrittoli added kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). and removed kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). labels Jun 30, 2023

afrittoli force-pushed the tep0137_tektonv1_move branch from 1e913d2 to 184309c Compare June 30, 2023 09:57

afrittoli force-pushed the tep0137_tektonv1_move branch from 184309c to 65b0eee Compare June 30, 2023 10:16

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 3, 2023

afrittoli force-pushed the tep0137_tektonv1_move branch 2 times, most recently from 9a851e0 to 3be4da5 Compare July 3, 2023 12:33

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 3, 2023

afrittoli mentioned this pull request Jul 5, 2023

[TEP-0137] Rename customrun package #6884

Merged

8 tasks

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 5, 2023

afrittoli mentioned this pull request Jul 10, 2023

[TEP-0137] Rework the events controller cache #6914

Open

10 tasks

afrittoli force-pushed the tep0137_tektonv1_move branch from d4b181c to b9ac5d8 Compare July 10, 2023 12:25

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2023

afrittoli commented Jul 10, 2023

View reviewed changes

afrittoli added this to the Pipelines v0.51 milestone Jul 25, 2023

afrittoli modified the milestones: Pipelines v0.51, Pipelines v0.52 Aug 8, 2023

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2023

jerop modified the milestones: Pipelines v0.52, Pipelines v0.53 Sep 20, 2023

pritidesai modified the milestones: Pipelines v0.53 LTS, Pipelines v0.54 Oct 17, 2023

jerop modified the milestones: Pipelines v0.54, Pipelines v0.55 Nov 9, 2023

vdemeester modified the milestones: Pipelines v0.55, Pipelines v0.56 Dec 21, 2023

chitrangpatel modified the milestones: Pipelines v0.56, Pipelines v0.57 Jan 17, 2024

afrittoli modified the milestones: Pipelines v0.57, Pipelines v0.58 Feb 26, 2024

pritidesai removed this from the Pipelines v0.58 milestone Mar 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TEP-0137] Restructure customrun event controller #6889

[TEP-0137] Restructure customrun event controller #6889

afrittoli commented Jun 29, 2023 •

edited by vdemeester

Loading

tekton-robot commented Jun 29, 2023

afrittoli Jun 29, 2023

afrittoli Jun 29, 2023

tekton-robot commented Jun 29, 2023

afrittoli Jun 29, 2023

tekton-robot commented Jun 29, 2023

afrittoli commented Jun 30, 2023

afrittoli commented Jun 30, 2023

tekton-robot commented Jun 30, 2023

tekton-robot commented Jun 30, 2023

tekton-robot commented Jun 30, 2023

tekton-robot commented Jun 30, 2023

tekton-robot commented Jun 30, 2023

tekton-robot commented Jul 3, 2023

tekton-robot commented Jul 3, 2023

afrittoli Jul 10, 2023

tekton-robot commented Jul 10, 2023

tekton-robot commented Jul 10, 2023

tekton-robot commented Aug 8, 2023

chitrangpatel commented Jan 10, 2024

vdemeester commented Jan 24, 2024

pritidesai commented Mar 20, 2024

[TEP-0137] Restructure customrun event controller #6889

Are you sure you want to change the base?

[TEP-0137] Restructure customrun event controller #6889

Conversation

afrittoli commented Jun 29, 2023 • edited by vdemeester Loading

Dependencies:

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jun 29, 2023

afrittoli Jun 29, 2023

Choose a reason for hiding this comment

afrittoli Jun 29, 2023

Choose a reason for hiding this comment

tekton-robot commented Jun 29, 2023

afrittoli Jun 29, 2023

Choose a reason for hiding this comment

tekton-robot commented Jun 29, 2023

afrittoli commented Jun 30, 2023

afrittoli commented Jun 30, 2023

tekton-robot commented Jun 30, 2023

tekton-robot commented Jun 30, 2023

tekton-robot commented Jun 30, 2023

tekton-robot commented Jun 30, 2023

tekton-robot commented Jun 30, 2023

tekton-robot commented Jul 3, 2023

tekton-robot commented Jul 3, 2023

afrittoli Jul 10, 2023

Choose a reason for hiding this comment

tekton-robot commented Jul 10, 2023

tekton-robot commented Jul 10, 2023

tekton-robot commented Aug 8, 2023

chitrangpatel commented Jan 10, 2024

vdemeester commented Jan 24, 2024

pritidesai commented Mar 20, 2024

afrittoli commented Jun 29, 2023 •

edited by vdemeester

Loading