⚠️ Implement Patch method

[adracus]

This adds the Patch method to the Client interface as well as to the two implementations (typedClient, unstructuredClient).

[adracus]

/assign @DirectXMan12

[k8s-ci-robot]

@adracus: GitHub didn't allow me to assign the following users: grantr.

Note that only kubernetes-sigs members and repo collaborators can be assigned.
For more information please see the contributor guide

In response to this:

/assign @DirectXMan12 @grantr

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.

[DirectXMan12]

/hold

Unfortunately, server-side apply hasn't landed yet, but I still feel like this is a really awkward structure to expose to users, especially as one that we have to support into perpetuity.

[DirectXMan12]

/priority important-soon

had a few more people ask about this. I think we may need to go forward with some patch-like thing.

[adracus]

@DirectXMan12 do you have more input on this? I'd be up to change this PR to suit these needs.

[coderanger]

I think it would be better to make this a splat of PatchOptions like we have for ListOptions and DeleteOptions to allow for future expansion. We can then create an option method for subresource.

[DirectXMan12]

our past discussed subresource plan has been a .Subresource method, but I'm open for debate on that. Otherwise agree that using a splat for subresource isn't the right way to go

[adracus]

I removed the subresources splat and exchanged it for PatchOptionFunc. However, since there was no final clarity yet how subresources are handled, the PatchOptions are empty for now.
When we finally agree on how to do subresource handling in the future, we can incorporate it without changing the API.

[DirectXMan12]

Ok, so, to move forward:

• no splat for subresource (see comment)
• move patch type and data into a single type that's an interface. Something along the lines of

type Patch interface {
    Type() types.PatchType
    Data(obj runtime.Object) []byte
}

This way, we can provide nicer abstractions on top of the patch method (e.g. passing []jsonpatch.Operation instead of raw bytes, just using client.Apply instead of having to construct an apply patch, etc.

Also (this is a bit more just preference), I feel it looks more natural to have object then patch data for the arguments order.

/hold cancel

Sorry for all the foot-dragging on this :-(

[adracus]

I implemented the spec almost as you described it:
For the Patch interface I removed the obj *runtime.Object part, since then in the implementation of it you'd have to do nasty casting again - see the discussion we had on controllerutil.CreateOrUpdate.

To also show that this approach is viable, I went ahead and implemented a CreateMergePatch method in controllerutil in which you see how to create patches without requiring the obj argument to the Data method.

[DirectXMan12]

This is a breaking change since it modifies an interface, so I've edited the title.

For the Patch interface I removed the obj *runtime.Object part,

I put the runtime.Object part in there so that we could cleanly have server-side apply work nicely:

cl.Patch(ctx, &desiredObject, client.Apply)

It also means that you can do stuff like

cl.Patch(ctx, &desiredObject, client.MergedFromOld(&oldObject))

Then, client.Apply should look something like

type applyPatcher struct {}
func (p applyPatcher) Type() types.PatchType { return types.ApplyPatchType /* or whatever it actually is */ }
func (p applyPatcher) Data(obj runtime.Object) ([]byte, error) {
    return runtime.Encode(unstructured.UnstructuredJSONScheme, obj) /* maybe should actually be an encoder and not the unstructured scheme, but doesn't really matter too much */
}

[adracus]

Alright I tried to reflect these changes, PTAL

[DirectXMan12]

Looks good! I'll follow up with some more patch types (json, server-side apply).

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adracus, 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:

• OWNERS [DirectXMan12]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[DirectXMan12]

pushed some fixes + options support, so we can get this merged.

[DirectXMan12]

I'll do the following in a follow-up PR:

• merge patch --> strategic merge patch (you generally want the latter)
• server side apply (you actually want this if available)

[DirectXMan12]

oof, that was a weird flake

[mengqiy]

/lgtm