⚠ Client Server side apply #2702
Conversation
Signed-off-by: Troy Connor <troy0820@users.noreply.github.com>
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: troy0820 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/L
|
/test all |
|
@troy0820: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
This feels wrong, but may be right? Any feedback is definitely appreciated. I do know that the test failed on core object without the type meta because I didn't get the gvk of the client.Object before converting, however this isn't tested against a CRD. Also, not sure if we want to allow applyconfigurations or just deal with the unstructured parts we are tryin to "apply". Didn't want to try to resolve this myself as this may need discussion. |
| @@ -347,6 +348,21 @@ func (c *client) Patch(ctx context.Context, obj Object, patch Patch, opts ...Pat | |||
| } | |||
| } | |||
|
|
|||
| func (c *client) Apply(ctx context.Context, obj Object, fieldOwner string) error { | |||
There was a problem hiding this comment.
Pretty heavily opposed to do anything in here before kubernetes/kubernetes#118138 is done because:
- The only type that can be used in here is
unstructured.Unstructured - The converting as you do it down below doesn't fix the issue that the normal types end up putting zero values in fields
- Which is the reason why applytypes exist, but you can not actually use applytypes here, because they do not fulfill
client.Object
/hold
There was a problem hiding this comment.
Definitely hold, and no strong opinions on the feedback beyond understanding the gaps that exists. Waiting on this make sense and provided the support going forward, strategy around this seems crucial. Here for that conversation.
There was a problem hiding this comment.
The whole topic boils down to "someone needs to do the work":
- The upstream issue I mentioned needs fixing, upstream signaled that they are ok with the idea but no one has shown up to actually put the work in
- This PR needs to be finished: [WIP] ✨ Generation of typed apply clients using upstream generator controller-tools#818
- A change like this in controller-runtime that adds an
Applymethod that only accepts apply types
k8s-ci-robot
added
the
do-not-merge/hold
This is an attempt at server side apply being applied to the
client.Writerinterface.