client-go: factor the dynamic client similarly to others #112774
Conversation
k8s-ci-robot
added
release-note-none
|
cc @ncdc |
| type dynamicClient struct { | ||
| client *rest.RESTClient | ||
| type DynamicClient struct { | ||
| client rest.Interface |
There was a problem hiding this comment.
+1 on switching this to an interface (and I think +1 on having a constructor that can construct a dynamic client passing in a rest.Interface... though I haven't thought through the use there a lot)
All other clients: - expose a New() method that takes a rest.Interface - expose their RESTClient() - return pointers to the type, not instances of an interface that the type implements For code that is generic over all Kubernetes clients, and for general developer experience, it's best to make sure that this client adheres to these common practices. Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
stevekuznetsov
force-pushed
the
skuznets/dynamic-client-similar
branch
from
12f5944
to
74af6f1
Compare
|
@liggitt thanks for the review - updated |
|
I didn't check if there is other consequences, but this looks consistent with the generated clients, and sounds like the right thing to do |
|
Existing users would have had to write code expecting |
|
I like the constructor that passes in rest.Interface, I'm less clear on the benefits of changing the return type of the existing constructor and exporting the type |
|
As noted in the commit body - the generated code is ripe for the use of Go generics. When one client of all of them looks different, writing abstractions around them becomes hard. Specifically here, allocating for a struct pointer vs an interface gets hairy, etc. I'll maybe flip the question back around: why should this client (and its' constructors) be special and different? |
|
I can't see that this can break, are we 100% sure? |
|
Today, the library returns the interface. With the change, the library returns a struct pointer that implements the interface. Since no concrete type was returned in the past, users could only assert that the return type was the interface, or some subset of the interface. Returning the concrete type cannot impact this, as the type implements all possible interfaces that users could have written. |
|
Returning the concrete type is also a suggested practice (https://github.com/golang/go/wiki/CodeReviewComments#interfaces). If there is any fallout to external consumers from this change, it is presumably a simple find-and-replace to correct all issues. I'm 👍 for this change. |
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
|
@liggitt any other thoughts? |
|
yeah, this lgtm now |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, stevekuznetsov 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 |
k8s-ci-robot
added
the
approved
|
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
All other clients:
For code that is generic over all Kubernetes clients, and for general developer experience, it's best to make sure that this client adheres to these common practices.
Signed-off-by: Steve Kuznetsov skuznets@redhat.com
/kind cleanup
/sig cli
/cc @liggitt @aojea