Bug 1654033 - Better default for patch type against custom resource records
Summary: Better default for patch type against custom resource records
Keywords:
Status: CLOSED WONTFIX
Alias: None
Product: OpenShift Container Platform
Classification: Red Hat
Component: oc
Version: 3.11.0
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
: ---
Assignee: Juan Vallejo
QA Contact: Xingxing Xia
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2018-11-27 21:16 UTC by Aleksandar Kostadinov
Modified: 2018-11-29 12:23 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-11-28 15:55:31 UTC
Target Upstream Version:


Attachments (Terms of Use)

Description Aleksandar Kostadinov 2018-11-27 21:16:58 UTC
Description of problem:
Default type for `oc patch` is `strategic` but it fails for CRD as described in bug 1619869.

As a user I would like to write `oc patch` without specifying type in most use cases.

Thus I think default patch type can be "merge" when run against a CRD. `oc` can figure out this is a custom resource and use "merge" as a default. In case I have specified --type=strategic, then command should fail though.

Version-Release number of selected component (if applicable):
oc v3.11.28

How reproducible:
always

Steps to Reproduce:
1. oc patch  prometheusrule/prometheus-k8s-rules  -n openshift-monitoring --type=strategic -p '{"metadata": {"annotations": {"cr_patch": "yep"}}}'

Actual results:

> Error from server (UnsupportedMediaType): the body of the request was in an unknown format - accepted media types include: application/json-patch+json, application/merge-patch+json

Expected results:

> patched

Comment 1 Juan Vallejo 2018-11-28 15:55:31 UTC
Figuring out whether a command is dealing with a specific resource type goes against our design of having "generic" commands. Although strategic patch types are not currently supported for CRDs, they might one day be supported. I would be against modifying the behavior of the patch command based on this temporary edge-case.

Comment 2 Aleksandar Kostadinov 2018-11-28 16:10:17 UTC
It is more and more common use case I think. In any case, can't we then modify server so that strategic patch on CRDs simply results in merge patch?

Comment 3 Juan Vallejo 2018-11-28 16:12:25 UTC
> In any case, can't we then modify server so that strategic patch on CRDs simply results in merge patch?

I think this would make sense. Adding Stefan for his thoughts here.

Comment 4 Stefan Schimanski 2018-11-29 07:34:08 UTC
The semantics of the available patch types is different. We cannot just switch to another one for CRDs on the server. The patch has a MIME type and that's what we follow when patching. Kubectl tells the server about the MIME type. It would violate the API specification to ignore that.

kubectl does not know and should not know whether a resource is backed by a CRD.

With the same argument that the merge type makes a behaviour difference, the user of kubectl has to consider carefully which one to choose. Otherwise he gets inconsistent results. I don't think we can or should take this burden from the user.

Strategic patch is considered as a failure by many (CRDs will not get suppport for it) and one might argue that it was a bad choice to set that as default. It's unfortunate that CRDs do not support it. We might want a better error message suggesting to switch to merge patch.

Comment 5 Aleksandar Kostadinov 2018-11-29 08:35:57 UTC
Stefan, `strategic` patch works mostly like `merge` patch. So on the server level it makes sense to me for resources that do not have `strategic` patch support to just do merge patch (as an internal "strategy").

I'm not sure I manage to explain it. i.e. instead of server returning that there is no strategy and fail, server can just use the "merge" strategy. Does what I'm saying make sense?

This would give convenience for users now as well allow strategies to be added going forward for whatever resources. e.g. even though we do not anticipate it, some day CRDs may have the metadata needed for merge patch. Then nothing will need to change on the client side, just on server side things will work better.

Comment 6 Stefan Schimanski 2018-11-29 12:04:15 UTC
Changing the default from one value to another is a breaking change. So you cannot do that later-on. The switch from strategic – which fails not – to something else might be ok, technically.

But in general, I don't like such an automatism of choosing different semantics depending on some heuristic. Kubectl and the API should work consistently for all resources. Making incompatible exception for certain resources is the wrong direction.

Why not just make the error message better and require to pass the right patch format to kubectl? It's easy to do, clear for the user and consistent.

Comment 7 Aleksandar Kostadinov 2018-11-29 12:23:20 UTC
I meant on the server side. I understand why ot doesn't make much sense for such logic on client.


Note You need to log in before you can comment on or make changes to this bug.