Patch strategy should be configurable

[kennethito]

I was looking for a way to change the patch strategy here. https://github.com/coryodaniel/k8s/blob/6d2609174e8a69c167047d111eae3a303e57deb7/lib/k8s/client/runner/base.ex#L204

Was also curious as to why the default isn't strategic merge, which seems to be the kubectl default? https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/

Valid values https://raw.githubusercontent.com/kubernetes/kubernetes/master/api/openapi-spec/swagger.json

"patch": {
        "consumes": [
          "application/json-patch+json",
          "application/merge-patch+json",
          "application/strategic-merge-patch+json",
          "application/apply-patch+yaml"
        ],

[mruoss]

Hey @kennethito

Yes I do agree, this should be configurable.

I can't answer why strategic merge is not the default as patch was still implemented by @coryodaniel. I have implemented apply, hence that case statement. That being said, changing the default would be a breaking change, I'd like to avoid. But yes to making it configurable.

[mruoss]

Thanks for the PR #228, @kennethito. While the approach in #228 solves this issue, I'd rather keep the abstraction and not expose more HTTP communication details to run. I see the following approaches:

1. make Operation a protocol so that different operations can have their own data structure but offer the same API to create requests. This is a big refactoring, though.
2. add headers to the Operation struct.

I prefer the first approach but it's all but a quick win! Could go for variant 2. now and implement 1. later... Open for discussion.

[kennethito]

For number 2 are you thinking that many of the functions on K8s.Client now take an additional optional parameter that's passed into Operation.build? Something like the below, where Operation.build would look for a :headers options?

  def patch(%{} = resource, opts \\ []), do: Operation.build(:patch, resource, opts)

Or something more along the lines of

operation |> K8s.Operation.put_header_param(:"Custom", "Header")

similar to #229 ?

[mruoss]

I was thinking about an API like this.

# Defined in operation.ex
@spec patch_type :: :strategic_merge | :merge | :json_merge | :apply

# Defined in client.ex
@spec patch(resource :: map(), type :: Operation.patch_type()) :: Operation.t()
def patch(resource, type \\ :merge) do
  Operation.build(:patch, resource, patch_type: type)
end

Once the patch types are implemented, K8s.Client.apply/2 could be implemented like this:

@spec apply(resource :: map(), mgmt_params :: keyword()) :: Operation.t()
def apply(resource, mgmt_params \\ []) do
  field_manager = Keyword.get(mgmt_params, :field_manager, @mgmt_param_defaults[:field_manager])
  force = Keyword.get(mgmt_params, :force, @mgmt_param_defaults[:force])
  Operation.build(:patch, resource, patch_type: :apply, field_manager: field_manager, force: force)
end

[kennethito]

Just back from some work travel and getting started again.

There exists

• K8s.Client.patch/1
• K8s.Client.patch/2
• K8s.Client.patch/4

So adding an optional patch_type to them all conflicts. The conflict is between /1 and /2, so I could do something like not add patch_type to K8s.Client.patch/1. How do you want to proceed?

I pushed a commit into #229 with my understanding of what you want (might be flawed), can you take a quick peek? If the approach looks good, I'll fix all the tests and get it ready for review.