-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
⚠️ Refactor source/handler/predicate packages to remove dep injection #2120
⚠️ Refactor source/handler/predicate packages to remove dep injection #2120
Conversation
419e32a
to
50a8e51
Compare
@@ -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=$? |
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.
These were actually useful to catch errors right away, and have a bit more verbosity when tests are running, we should probably keep them
/assign @alvaroaleman |
Watches(&source.Kind{Type: &appsv1.ReplicaSet{}}, &handler.EnqueueRequestForObject{}). | ||
Watches(source.Kind(m.GetCache(), &appsv1.ReplicaSet{}), &handler.EnqueueRequestForObject{}). |
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.
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
Outdated
type kindWithCache struct { | ||
kind Kind | ||
// Kind creates a KindSource with the given cache provider. | ||
func Kind(cache cache.Cache, object client.Object) KindSource { |
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.
Can we use the same signature as NewKindWithCache did (i.E. reverse the arguments) so updating code for this becomes less annoying?
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.
We can but the EnqueueRequestForOwner counterpart for example takes any required parameters before taking the object, wdyt?
@@ -242,15 +241,6 @@ type and struct { | |||
predicates []Predicate | |||
} | |||
|
|||
func (a and) InjectFunc(f inject.Func) error { |
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.
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
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.
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?
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.
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
pkg/builder/controller.go
Outdated
typeForSrc, err := blder.project(srckind.Type, w.objectProjection) | ||
if err != nil { | ||
return err | ||
if srckind, ok := w.src.(source.KindSource); ok { |
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.
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
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.
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
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.
I have a related comment below. not sure if we can actually do this as is #2120 (comment)
d8d9d48
to
5eeee55
Compare
/test pull-controller-runtime-test-master |
5eeee55
to
b9f5137
Compare
pkg/builder/controller.go
Outdated
@@ -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. |
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.
nit: Comment seems to be outdated
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.
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.")
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.
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.
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.
PTAL, added a new interface called TypedSource
which lets users provide their own if needed
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.
lgtm
d990052
to
cf8df4b
Compare
Signed-off-by: Vince Prignano <[email protected]>
e1c8196
to
ea1fcf3
Compare
/hold cancel |
[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:
Approvers can indicate their approval by writing |
/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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Vince Prignano [email protected]