Patch strategy should be configurable
kennethito opened this issue · 5 comments
I was looking for a way to change the patch strategy here.
k8s/lib/k8s/client/runner/base.ex
Line 204 in 6d26091
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"
],
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.
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:
- 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. - 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.
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 ?
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
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.