⚠️ Refactor source/handler/predicate packages to remove dep injection #2120

vincepri · 2023-01-06T20:50:12Z

Signed-off-by: Vince Prignano vince@prigna.com

vincepri · 2023-01-06T22:18:48Z

hack/test-all.sh

@@ -25,7 +25,7 @@ if [[ -n ${ARTIFACTS:-} ]]; then
 fi

 result=0
-go test -race ${P_FLAG} ${MOD_OPT} ./... ${GINKGO_ARGS} || result=$?
+go test -v -race ${P_FLAG} ${MOD_OPT} ./... --ginkgo.fail-fast ${GINKGO_ARGS} || result=$?


These were actually useful to catch errors right away, and have a bit more verbosity when tests are running, we should probably keep them

vincepri · 2023-01-06T22:20:53Z

/assign @alvaroaleman

vincepri · 2023-01-06T22:25:28Z

pkg/builder/controller_test.go

-				Watches(&source.Kind{Type: &appsv1.ReplicaSet{}}, &handler.EnqueueRequestForObject{}).
+				Watches(source.Kind(m.GetCache(), &appsv1.ReplicaSet{}), &handler.EnqueueRequestForObject{}).


This is now the new experience on how to create a controller in a manager. Over time, the builder has increased the options we're passing in, which is ok, but we've also had to deal with all of the dependency injection stuff which isn't pretty at all.

This UX might be a stopgap, but it's actually nicer to be explicit where a source is getting its cache or any other function. In a follow-up PR we might want to explore easier ways to plow through this information when we're clearly building a controller in a Manager, and make the source aware that it can access whatever it needs through the manager.

For now, we can keep this as a first step breaking change and iterate on it as we see fit. The clarity might come handy in the future when we reason through the codebase.

pkg/source/source.go

alvaroaleman · 2023-01-07T17:49:06Z

pkg/source/source.go

-type kindWithCache struct {
-	kind Kind
+// Kind creates a KindSource with the given cache provider.
+func Kind(cache cache.Cache, object client.Object) KindSource {


Can we use the same signature as NewKindWithCache did (i.E. reverse the arguments) so updating code for this becomes less annoying?

We can but the EnqueueRequestForOwner counterpart for example takes any required parameters before taking the object, wdyt?

pkg/source/source.go

alvaroaleman · 2023-01-07T17:52:54Z

pkg/predicate/predicate.go

@@ -242,15 +241,6 @@ type and struct {
 	predicates []Predicate
 }

-func (a and) InjectFunc(f inject.Func) error {


Can we keep the instances where things just pass on the injet.Func for one release and log a deprecation warning? Otherwise this will lead to runtime failures

~~This would receive a compiler error, I'd rather start removing all of dependency injection methods going forward, they have been deprecated for a while, same goes for the SetFields structs~~

I think I originally misread your comment, let me think about it a bit more. Should we just panic in every place we have these instead of returning errors?

I went ahead and removed most of the injection code except for the ones that touches webhooks, which we should handle in a separate PR

alvaroaleman · 2023-01-07T17:58:37Z

pkg/builder/controller.go

-			typeForSrc, err := blder.project(srckind.Type, w.objectProjection)
-			if err != nil {
-				return err
+		if srckind, ok := w.src.(source.KindSource); ok {


That we need this is pretty nonsensical to me. Builder.Watches takes a source and might get an arg for object projection and then we manipulate the source? That doesn't make much sense to me, ppl should just create the Source correctly in the first place

I'm with you that this isn't ideal, although the UX for building sources with PartialObjectMetadata is pretty poor if we don't do this for our users. That would basically mean creating an PartialObjectMetadata, figure out the GVK, potentially using the scheme directly, which can cause errors. We should probably keep this as-is for now, and figure out a different way later

I have a related comment below. not sure if we can actually do this as is #2120 (comment)

vincepri · 2023-01-09T20:05:30Z

/test pull-controller-runtime-test-master

sbueringer · 2023-01-12T15:03:06Z

pkg/builder/controller.go

@@ -259,12 +255,12 @@ func (blder *Builder) doWatch() error {
 		allPredicates = append(allPredicates, w.predicates...)

 		// If the source of this watch is of type *source.Kind, project it.


nit: Comment seems to be outdated

Q: is it correct to check for SyncingSource and then pass the src into KindAsPartialMetadata where we check for kind?

Could it be a SyncingSource which is not kind? ("Users may build their own Source implementations.")

I agree it's not ideal. Originally I had a KindSource instead with AsPartialMetadata() method required. These builders need a lot of rework honestly, the UX also isn't ideal with modifiers in place. I'm happy to go back to the interface if we prefer that.

PTAL, added a new interface called TypedSource which lets users provide their own if needed

pkg/cluster/cluster_test.go

pkg/handler/enqueue_owner.go

pkg/source/source.go

pkg/source/source_test.go

Signed-off-by: Vince Prignano <vincepri@redhat.com>

vincepri · 2023-01-18T15:36:12Z

/hold cancel

k8s-ci-robot · 2023-01-18T15:38:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, vincepri

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:

~~OWNERS~~ [alvaroaleman,vincepri]

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

vincepri · 2023-01-18T15:42:48Z

/retest

https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.15.0 * Make `*http.Client` configurable and use/share the same client by default kubernetes-sigs/controller-runtime#2122 * Remove dependency injection functions kubernetes-sigs/controller-runtime#2134, kubernetes-sigs/controller-runtime#2120 * Add context to EventHandler(s) kubernetes-sigs/controller-runtime#2139 * `Validator` and `CustomValidator` interfaces have been modified to allow returning warnings kubernetes-sigs/controller-runtime#2014 * operator-framework is also pinned to ecb9be48837 until a new release is cut supporting controller-runtime v0.15.0 Signed-off-by: Lee Yarwood <lyarwood@redhat.com>

…tion" kubernetes-sigs/controller-runtime#2120

…tion` kubernetes-sigs/controller-runtime#2120

kubernetes-sigs/controller-runtime#2120

https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.15.0 * Make `*http.Client` configurable and use/share the same client by default kubernetes-sigs/controller-runtime#2122 * Remove dependency injection functions kubernetes-sigs/controller-runtime#2134, kubernetes-sigs/controller-runtime#2120 * Add context to EventHandler(s) kubernetes-sigs/controller-runtime#2139 * `Validator` and `CustomValidator` interfaces have been modified to allow returning warnings kubernetes-sigs/controller-runtime#2014 * operator-framework is also pinned to ecb9be48837 until a new release is cut supporting controller-runtime v0.15.0 Signed-off-by: Lee Yarwood <lyarwood@redhat.com>

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 6, 2023

k8s-ci-robot requested review from alvaroaleman and joelanford January 6, 2023 20:50

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 6, 2023

vincepri changed the title ~~WIP: Refactor source/handler/predicate packages to remove dep injection~~ ⚠️ WIP: Refactor source/handler/predicate packages to remove dep injection Jan 6, 2023

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 6, 2023

vincepri force-pushed the rework-source-predicate-handlers branch 4 times, most recently from 419e32a to 50a8e51 Compare January 6, 2023 22:15

vincepri changed the title ~~⚠️ WIP: Refactor source/handler/predicate packages to remove dep injection~~ ⚠️ Refactor source/handler/predicate packages to remove dep injection Jan 6, 2023

vincepri commented Jan 6, 2023

View reviewed changes

k8s-ci-robot assigned alvaroaleman Jan 6, 2023

vincepri commented Jan 6, 2023

View reviewed changes

alvaroaleman reviewed Jan 7, 2023

View reviewed changes

vincepri force-pushed the rework-source-predicate-handlers branch 2 times, most recently from d8d9d48 to 5eeee55 Compare January 9, 2023 18:01

vincepri force-pushed the rework-source-predicate-handlers branch from 5eeee55 to b9f5137 Compare January 11, 2023 15:33

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 11, 2023

sbueringer reviewed Jan 12, 2023

View reviewed changes

vincepri force-pushed the rework-source-predicate-handlers branch 5 times, most recently from d990052 to cf8df4b Compare January 12, 2023 16:17

vincepri force-pushed the rework-source-predicate-handlers branch from 2bfe2e1 to e1c8196 Compare January 18, 2023 15:25

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 18, 2023

alvaroaleman approved these changes Jan 18, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 18, 2023

Remove SetFields from Manager and Controller

ea1fcf3

Signed-off-by: Vince Prignano <vincepri@redhat.com>

vincepri force-pushed the rework-source-predicate-handlers branch from e1c8196 to ea1fcf3 Compare January 18, 2023 15:34

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 18, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 18, 2023

alvaroaleman approved these changes Jan 18, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 18, 2023

k8s-ci-robot merged commit 9241bce into kubernetes-sigs:master Jan 18, 2023

This was referenced May 5, 2023

Update k8s dependencies to v0.27.2 and controller-runtime to v0.15.0 operator-framework/operator-lib#114

Merged

Bump k8s to v0.27.2 kubevirt/hyperconverged-cluster-operator#2333

Merged

ary1992 mentioned this pull request Jun 16, 2023

Upgrade k8s.io/* to v0.27, sigs.k8s.io/controller-runtime to v0.15 gardener/gardener#7991

Closed

ary1992 added a commit to ary1992/gardener that referenced this pull request Jun 28, 2023

Adapt "Refactor source/handler/predicate packages to remove dep injec…

0d2ea18

…tion" kubernetes-sigs/controller-runtime#2120

ary1992 added a commit to ary1992/gardener that referenced this pull request Jun 28, 2023

Adapt `Refactor source/handler/predicate packages to remove dep injec…

205346f

…tion` kubernetes-sigs/controller-runtime#2120

ary1992 added a commit to ary1992/gardener that referenced this pull request Jun 28, 2023

Adapt handler.EnqueueRequestForOwner signature

3cff0a2

kubernetes-sigs/controller-runtime#2120

ary1992 mentioned this pull request Jul 7, 2023

Remove dependency injection functions gardener/gardener#8217

Merged

duanhongyi mentioned this pull request Aug 7, 2023

bump sigs.k8s.io/controller-runtime to v0.15.1 drycc-addons/service-catalog#25

Closed

lyarwood mentioned this pull request Aug 9, 2023

chore: Update controller-runtime to v0.15.0 kubevirt/ssp-operator#604

Closed

Neo2308 mentioned this pull request Aug 13, 2023

Update k8s libs to 1.27.2 operator-framework/operator-sdk#6542

Closed

10 tasks

nakamasato mentioned this pull request Sep 8, 2023

controller-runtime: update README.md nakamasato/kubernetes-training#280

Merged

maqiuyujoyce mentioned this pull request Dec 27, 2023

Upgrade kdp to v0.15.0-beta.1 GoogleCloudPlatform/k8s-config-connector#1038

Merged

1 task

GuptaManan100 mentioned this pull request Dec 28, 2023

Upgrade Kubernetes and controller-runtime dependencies planetscale/vitess-operator#516

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ Refactor source/handler/predicate packages to remove dep injection #2120

⚠️ Refactor source/handler/predicate packages to remove dep injection #2120

vincepri commented Jan 6, 2023

vincepri Jan 6, 2023

vincepri commented Jan 6, 2023

vincepri Jan 6, 2023

alvaroaleman Jan 7, 2023

vincepri Jan 9, 2023

alvaroaleman Jan 7, 2023

vincepri Jan 9, 2023 •

edited

vincepri Jan 11, 2023

alvaroaleman Jan 7, 2023

vincepri Jan 9, 2023

sbueringer Jan 12, 2023

vincepri commented Jan 9, 2023

sbueringer Jan 12, 2023

sbueringer Jan 12, 2023

vincepri Jan 12, 2023

vincepri Jan 12, 2023

sbueringer Jan 12, 2023

vincepri commented Jan 18, 2023

k8s-ci-robot commented Jan 18, 2023

vincepri commented Jan 18, 2023

		Watches(&source.Kind{Type: &appsv1.ReplicaSet{}}, &handler.EnqueueRequestForObject{}).
		Watches(source.Kind(m.GetCache(), &appsv1.ReplicaSet{}), &handler.EnqueueRequestForObject{}).

		@@ -259,12 +255,12 @@ func (blder *Builder) doWatch() error {
		allPredicates = append(allPredicates, w.predicates...)

		// If the source of this watch is of type *source.Kind, project it.

⚠️ Refactor source/handler/predicate packages to remove dep injection #2120

⚠️ Refactor source/handler/predicate packages to remove dep injection #2120

Conversation

vincepri commented Jan 6, 2023

Choose a reason for hiding this comment

vincepri commented Jan 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincepri Jan 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincepri commented Jan 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincepri commented Jan 18, 2023

k8s-ci-robot commented Jan 18, 2023

vincepri commented Jan 18, 2023

vincepri Jan 9, 2023 •

edited