Use CR unstructured client for dynamic objects#137
Conversation
No longer need to create dynamic clients. CR will do this for us internally based on the TypeMeta of the unstructured object. Also updates tests to call the interface Addressable instead of Sinkable.
knative-prow-robot
added
the
size/XL
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: grantr The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
| Reconcile(ctx context.Context, object runtime.Object) (runtime.Object, error) | ||
| InjectClient(c client.Client) error | ||
| //TODO(grantr) make these separate interfaces | ||
| InjectConfig(c *rest.Config) error |
There was a problem hiding this comment.
Does anything need this anymore? I thought it was only used for the dynamic client.
There was a problem hiding this comment.
I had originally pulled this out into a separate PR, but it was complicated to make this change separately. Further commits forthcoming.
|
/hold |
knative-prow-robot
added
the
do-not-merge/hold
Can use the standard CR client now that it supports unstructured objects.
CR provides interfaces and helpers to detect when injection is possible.
knative-prow-robot
removed
the
do-not-merge/hold
|
|
||
| type KnativeReconciler interface { | ||
| Reconcile(ctx context.Context, object runtime.Object) (runtime.Object, error) | ||
| InjectClient(c client.Client) error |
There was a problem hiding this comment.
I'm surprised this line can be removed. Are any of these reconcilers viable without a client?
There was a problem hiding this comment.
The inject.ClientInto method will detect if the reconciler implements InjectClient and call it if so.
There's no need to require this method in the interface because there are other ways of injecting the client. For example, in pkg/controller/containersource/provider.go:
p := &sdk.Provider{
AgentName: controllerAgentName,
Parent: &v1alpha1.ContainerSource{},
Owns: []runtime.Object{&appsv1.Deployment{}},
Reconciler: &reconciler{
recorder: mgr.GetRecorder(controllerAgentName),
scheme: mgr.GetScheme(),
+ client: mgr.GetClient(),
},
}There was a problem hiding this comment.
So my preference is to continue to require it in the interface. You are right that there are other ways to do it, but do they provide any advantages?
If an author forgets to somehow inject the client (via whichever method they prefer), then this controller will fail at runtime. And even then, only after the controller is trying to do something. I'm a fan of keeping this a compile-time error 😄
There was a problem hiding this comment.
I guess the advantage of the field setter is that it's slightly more obvious when the client value is set and where it comes from. I don't have a strong preference either way though, so I added it back.
This is intended to make forgetting to inject a client a compile error instead of a runtime error.
| @@ -216,7 +214,5 @@ func (r *reconciler) InjectClient(c client.Client) error { | |||
| } | |||
|
|
|||
| func (r *reconciler) InjectConfig(c *rest.Config) error { | |||
|
The following is the coverage report on pkg/.
|
The controllers written before knative#137 are reverted to use the dynamic client. More recent controllers cronjobsource and awssqssource use the controller-runtime client still, since there was never a version of them that used the dynamic client.
4 participants
Since kubernetes-sigs/controller-runtime#101, controller-runtime will create and manage dynamic clients when given
unstructured.Unstructuredobjects with a filled inTypeMeta. Using this allows us to eliminate the dynamic clients created to deal with Addressable objects.Also updates tests to call the interface Addressable instead of Sinkable.