✨ Add client.StrategicMergeFrom

[timebertt]

Fixes #1375

This PR adds client.StrategicMergeFrom.
The function is similar to client.{MergeFrom,MergeFromWithOptions}, but it creates a strategic-merge-patch instead of a merge-patch. See doc strings for more details about the differences of the two patch types and to figure out when to use which one.

Note, that strategic merge patches are not supported by CRDs.

[k8s-ci-robot]

Hi @timebertt. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[coderanger]

Is there any way to refactor this so it's not quite as much copy-paste from MergeFrom? It feels like a lot of duplication.

[timebertt]

@coderanger Yes, you're right. It turned out to be very much the same like MergeFrom.
Deduplicated it now. PTAL.

[cbandy]

Is it appropriate to use MergeFromOption here? My concern is that merge-patch and strategic-merge-patch could diverge in the options they take in the future.

[timebertt]

I was contemplating about the same, glad you bring it up here.

The reason I chose this way, is that if options specific to either patch type will be added in the future, we could simply add another interface StrategicMergeFromOption, implement both interfaces in the options that we already have and just switch the parameter type in StrategicMergeFrom. This way, folks using the options wouldn't get any compile tim error when upgrading.
And as it's not required to differentiate right now, I chose to reuse MergeFromOption for simplicity's sake.

Let me know, if you have any concerns with this approach.

[alvaroaleman]

Can you please add godocs here and to MergeFrom that explain how StrategicMergePatch and MergePatch differ and when to use which?

[timebertt]

Sure, good idea. Done.
WDYT now?

[timebertt]

@timebertt: PR needs rebase.

Done.

[cbandy]

I see a failure now:

Patch
controller-runtime/pkg/client/client_test.go:3242
  StrategicMergeFrom
  controller-runtime/pkg/client/client_test.go:3307
    creates a strategic merge patch with the modifications applied during the mutation, using optimistic locking [It]
    controller-runtime/pkg/client/client_test.go:3357

    Unexpected error:
        <mergepatch.ErrPreconditionFailed>: {
            message: "precondition failed for: map[metadata:map[resourceVersion:10] spec:map[template:map[spec:map[$setElementOrder/containers:[map[name:main] map[name:sidecar]] containers:[map[image:foo:v2 name:main]]]]]]",
        }
        precondition failed for: map[metadata:map[resourceVersion:10] spec:map[template:map[spec:map[$setElementOrder/containers:[map[name:main] map[name:sidecar]] containers:[map[image:foo:v2 name:main]]]]]]
    occurred

    controller-runtime/pkg/client/client_test.go:3376

[timebertt]

@cbandy yes, sorry about that.
The preconditions and the respective test case are not needed anymore since your changes in #1395.
I fixed it accordingly. PTAL.

[alvaroaleman]

/ok-to-test

[alvaroaleman]

/lgtm
/approve

thank you!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, timebertt

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 [alvaroaleman]

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