🐛 respect context in unstructured client #812
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
cc @alvaroaleman for some additional 👀 |
There was a problem hiding this comment.
shawn-hurley mentioned there were previously some issues with this, but as-is the implementation passes all current tests?
Yeah and we have a fairly exhaustive set on integration tests for the client, except for UpdateStatus and PatchStatus, could you add those? If they pass as well I think we should be good.
We should probably still wait for Shawn to confirm that whatever issue he saw doesn't exist here :)
pkg/client/unstructured_client.go
Outdated
| NamespaceIfScoped(key.Namespace, r.isNamespaced()). | ||
| Resource(r.resource()). | ||
| Context(ctx). | ||
| Name(key.Name).Do().Into(obj) |
There was a problem hiding this comment.
Nit: Please keep the linebreaking the same as in the other methods
pkg/client/unstructured_client.go
Outdated
| "k8s.io/client-go/dynamic" | ||
| ) | ||
|
|
||
| // client is a client.Client that reads and writes directly from/to an API server. It lazily initializes | ||
| // new clients at the time they are used, and caches the client. | ||
| type unstructuredClient struct { | ||
| cache *clientCache | ||
| client dynamic.Interface | ||
| restMapper meta.RESTMapper |
There was a problem hiding this comment.
From what I can see, both the client and the restMapper are now unused
pkg/client/codec.go
Outdated
| "k8s.io/apimachinery/pkg/runtime/schema" | ||
| ) | ||
|
|
||
| type parameterCodec struct{} |
There was a problem hiding this comment.
Could we add a var _ runtime.ParamterCodec = parameterCodec to make this easier to understand? Also, please add some godocs to it clarifying how exactly it differs from the ParamterCodec obtained via the scheme
There was a problem hiding this comment.
I think a more correct approach to take is setting the serializer for unstructured on the restclient when we fetch it from the cache. I think that will allow us to remove this code and streamline further.
If that doesn't work out, i'll add some comments that this is for no-conversion serialization of create/delete/etc params.
For comparison sake, I looked at the dynamic client go client which forces everything to metav1. So it's technically correct right now due to metav1 being the only option, but setting the serializer is the more future-proof option.
e: seems like that breaks all sorts of things, which I suppose makes sense. sticking to the param codec. Maybe it makes sense to add a no-conversion param codec to unstructured in apimachinery directly?
There was a problem hiding this comment.
added a comment and renamed for clarity
|
Thanks for the review, good points 👍 I think I might be able to make the parameter codec a bit cleaner, still poking at it. If not I'll drop a comment explaining. For posterity, we chatted on slack and agreed this approach makes sense |
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| By("validating updated Deployment has type information") | ||
| Expect(u.GroupVersionKind()).To(Equal(depGvk)) |
There was a problem hiding this comment.
I think we should also get the deployment with the generated clientset and verify that its status got updated/patched 🙃
There was a problem hiding this comment.
added it for patch, I think it's there for the update calls already?
There was a problem hiding this comment.
not sure why they're organized this way, but I guess the tests are broken out fairly granular -- one test only does the type validation but the other two validate the actual status server side?
There was a problem hiding this comment.
Oh you are right. Indeed very granular. Well, that definitely helps when debugging :)
k8s-ci-robot
added
the
lgtm
|
/assign @DirectXMan12 I imagine you'd want to review this before merging |
|
LGTM as well |
|
awesome /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeldeib, DirectXMan12 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
|
/test pull-controller-runtime-test-master 50 millisecond timeout feels aggressive, not sure if there's something hiding causing a flake or if it's just a low threshold. I can't repro that failure locally anyway. |
When using controller-runtime to fetch the components, the version information was able to be inferred from GK. However, the latest controller-runtime requires the version infomation to be defined: kubernetes-sigs/controller-runtime#812. Hense, we need to provide the component's version.
ref: #41 (comment)
aligns the unstructured client with the typed client, sharing a cache between them for object metadata. plumbs context through to the restclient for unstructured client in doing so.
@shawn-hurley mentioned there were previously some issues with this, but as-is the implementation passes all current tests?
also, idk how hacky people consider this 😄 feels like I probably need to fix up some GVKs somewhere in the lists...