Description of problem: Behavior of `oc patch` when given a JSONPatch parameter is confusing. It reports that object is patched but in fact no modification to the underlying object is performed. See an example: > [root@ip-172-18-15-105 ~]# oc get scc privileged -o yaml > ... > users: > - system:serviceaccount:openshift-infra:build-controller > - system:serviceaccount:default:default > > [root@ip-172-18-15-105 ~]# oc patch scc privileged -p '{"op": "add", "path": "/users", "value": "system:admin"}' > "privileged" patched > > [root@ip-172-18-15-105 ~]# oc get scc privileged -o yaml > ... > users: > - system:serviceaccount:openshift-infra:build-controller > - system:serviceaccount:default:default I think the `oc patch` command should have a parameter to specify that the parameter is a JSONPatch (not a merge JSON), i.e. set rest request content type to `application/json-patch+json`, `application/merge-patch+json` or `application/strategic-merge-patch+json` as user desires. Also it may try to detect JSONPatch by itself as the format is pretty much obvious. But ultimately an error should be reported by CLI when operation dod not actually succeed. Probably it also depends on server side? I can confirm that regular merge-patch+json parameter works fine for the above use case. I know about `oadm policy add-scc-to-user` but still the bug holds true. Version-Release number of selected component (if applicable): oc v1.1.1-168-g5597bae kubernetes v1.1.0-origin-1107-g4c8e6f4 How reproducible: always
It looks like there's two parts to this issue/request: 1. `oc patch` (and equivalently `kubectl patch`) should accept an argument to denote the type of patch that is being used. This seems like a good feature to have, and should not be very difficult. 2. Submitting a patch in one format when using another should throw an error: This is a bit trickier, since it seems like what's happening at the server level is this: Suppose we're patching an object `{"a": 1}` with `{"op": "add", "path": "/a", "value": 2}`. Then, the server receives the patch, and attempts the merge patch (or strategic merge patch). The patch completes successfully, resulting in the JSON blob `{"a": 1, "op": "add", "path": "/a", "value": 2}` as the "patched" object. The object is then decoded from JSON back into a Go object, at which point the extraneous parts of the "patched" object are discarded, and then the object is updated in etcd. Thus, it appears that the patch goes through successfully. Ergo, it might be a bit trickier to provide this functionality.
Excellent explanation, thank you. But isn't it incorrect that decoding JSON back into a Go object causes extraneous parts of the "patched" object to be discarded instead of throwing an error? Current behavior appears to me as a good way to leave user unaware that the patch did not actually work properly. Failing on wrong JSON on the other hand gives a clear indication something was wrong at the point of the faulty operation so would be easier to debug.
> Excellent explanation, thank you. But isn't it incorrect that decoding JSON back into a Go object causes extraneous parts of the "patched" object to be discarded instead of throwing an error? AFAICT, this behavior is due to the way that Go's JSON decoder works. I agree that probably we should at least warn if this happens. I'll see if I can figure out a good way to work around this.
This needs to be an RFE for CLI to support more patch strategies.
this not a bug, please open an RFE if you want to advocate for this feature.