Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

content-type for patch commands #959

Closed
wants to merge 1 commit into from
Closed

content-type for patch commands #959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions examples/custom_object.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,19 @@
- name: v1
served: true
storage: true
schema:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The schema is now required by CRD.

openAPIV3Schema:
type: object
properties:
spec:
type: object
properties:
cronSpec:
type: string
image:
type: string
replicas:
type: integer
scope: Namespaced
names:
plural: crontabs
Expand Down Expand Up @@ -80,6 +93,28 @@ def main():
print("Resource details:")
pprint(resource)

# patch the resource
resource = api.patch_namespaced_custom_object(
group="stable.example.com",
version="v1",
name="my-new-cron-object",
namespace="default",
plural="crontabs",
body={'image': 'my-awesome-cron-image:2'},
content_type="application/merge-patch+json"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This parameter is not necessary - select_header_content_type can manage with it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Custom resources support JSON patch, so this parameter can be used to hint JSON patch / JSON merge patch, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about application/apply-patch+yaml? Why aren't we allowing to override from the caller?

)

# get the resource and print out data
resource = api.get_namespaced_custom_object(
group="stable.example.com",
version="v1",
name="my-new-cron-object",
namespace="default",
plural="crontabs",
)
print("Resource details:")
pprint(resource)

# delete it
api.delete_namespaced_custom_object(
group="stable.example.com",
Expand Down
14 changes: 10 additions & 4 deletions kubernetes/client/api/core_v1_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18250,6 +18250,7 @@ def patch_namespaced_config_map_with_http_info(self, name, namespace, body, **kw
all_params.extend(
[
'async_req',
'content_type',
'_return_http_data_only',
'_preload_content',
'_request_timeout'
Expand Down Expand Up @@ -18308,8 +18309,10 @@ def patch_namespaced_config_map_with_http_info(self, name, namespace, body, **kw
['application/json', 'application/yaml', 'application/vnd.kubernetes.protobuf']) # noqa: E501

# HTTP header `Content-Type`
header_params['Content-Type'] = self.api_client.select_header_content_type( # noqa: E501
['application/json-patch+json', 'application/merge-patch+json', 'application/strategic-merge-patch+json', 'application/apply-patch+yaml']) # noqa: E501
header_params['Content-Type'] = local_var_params.get('content_type',
self.api_client.select_header_content_type(['application/json-patch+json',
'application/merge-patch+json', 'application/strategic-merge-patch+json', 'application/apply-patch+yaml'],
'PATCH', body_params))

# Authentication setting
auth_settings = ['BearerToken'] # noqa: E501
Expand Down Expand Up @@ -20590,6 +20593,7 @@ def patch_namespaced_service_with_http_info(self, name, namespace, body, **kwarg
all_params.extend(
[
'async_req',
'content_type',
'_return_http_data_only',
'_preload_content',
'_request_timeout'
Expand Down Expand Up @@ -20648,8 +20652,10 @@ def patch_namespaced_service_with_http_info(self, name, namespace, body, **kwarg
['application/json', 'application/yaml', 'application/vnd.kubernetes.protobuf']) # noqa: E501

# HTTP header `Content-Type`
header_params['Content-Type'] = self.api_client.select_header_content_type( # noqa: E501
['application/json-patch+json', 'application/merge-patch+json', 'application/strategic-merge-patch+json', 'application/apply-patch+yaml']) # noqa: E501
header_params['Content-Type'] = local_var_params.get('content_type',
self.api_client.select_header_content_type(['application/json-patch+json',
'application/merge-patch+json', 'application/strategic-merge-patch+json', 'application/apply-patch+yaml'],
'PATCH', body_params))

# Authentication setting
auth_settings = ['BearerToken'] # noqa: E501
Expand Down
30 changes: 18 additions & 12 deletions kubernetes/client/api/custom_objects_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2336,6 +2336,7 @@ def patch_cluster_custom_object_with_http_info(self, group, version, plural, nam
all_params.extend(
[
'async_req',
'content_type',
'_return_http_data_only',
'_preload_content',
'_request_timeout'
Expand Down Expand Up @@ -2404,8 +2405,8 @@ def patch_cluster_custom_object_with_http_info(self, group, version, plural, nam
['application/json']) # noqa: E501

# HTTP header `Content-Type`
header_params['Content-Type'] = self.api_client.select_header_content_type( # noqa: E501
['application/merge-patch+json']) # noqa: E501
header_params['Content-Type'] = local_var_params.get('content_type', self.api_client.\
select_header_content_type(['application/merge-patch+json'], 'PATCH', body_params))

# Authentication setting
auth_settings = ['BearerToken'] # noqa: E501
Expand Down Expand Up @@ -2505,6 +2506,7 @@ def patch_cluster_custom_object_scale_with_http_info(self, group, version, plura
all_params.extend(
[
'async_req',
'content_type',
'_return_http_data_only',
'_preload_content',
'_request_timeout'
Expand Down Expand Up @@ -2573,8 +2575,8 @@ def patch_cluster_custom_object_scale_with_http_info(self, group, version, plura
['application/json', 'application/yaml', 'application/vnd.kubernetes.protobuf']) # noqa: E501

# HTTP header `Content-Type`
header_params['Content-Type'] = self.api_client.select_header_content_type( # noqa: E501
['application/merge-patch+json']) # noqa: E501
header_params['Content-Type'] = local_var_params.get('content_type', self.api_client.\
select_header_content_type(['application/merge-patch+json'], 'PATCH', body_params))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

want to double check, this PR would be backward compatible for both core native types, and custom resources, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's tricky question, in general it's backward compatible, but I can imagine that some calls can be interpreted differently by select_header_content_type - the current version usually uses the first patch method but this version tries to guess the content-type and results may be different... To be honest I don't think it's a real problem.

The positive aspect is that we will be able to stop removing the first element from the list as in this change roycaihw@61f1fbe and select_header_content_type should do the job.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good to me, @roycaihw do you have any concern?

Copy link
Member

@roycaihw roycaihw May 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to it's not a real problem (at least for this repo, given how we use select_header_content_type today).

Today native types can only use application/json-patch+json or application/strategic-merge-patch+json. With this change, existing applications will still get the same result. New applications can use the exposed content_type to choose using application/merge-patch+json, or even application/apply-patch+yaml

Custom resources today can only use application/merge-patch+json and will get the same result. With this change, applications will be able to pass in application/json-patch+json body.


# Authentication setting
auth_settings = ['BearerToken'] # noqa: E501
Expand Down Expand Up @@ -2674,6 +2676,7 @@ def patch_cluster_custom_object_status_with_http_info(self, group, version, plur
all_params.extend(
[
'async_req',
'content_type',
'_return_http_data_only',
'_preload_content',
'_request_timeout'
Expand Down Expand Up @@ -2742,8 +2745,8 @@ def patch_cluster_custom_object_status_with_http_info(self, group, version, plur
['application/json', 'application/yaml', 'application/vnd.kubernetes.protobuf']) # noqa: E501

# HTTP header `Content-Type`
header_params['Content-Type'] = self.api_client.select_header_content_type( # noqa: E501
['application/merge-patch+json']) # noqa: E501
header_params['Content-Type'] = local_var_params.get('content_type', self.api_client.\
select_header_content_type(['application/merge-patch+json'], 'PATCH', body_params))

# Authentication setting
auth_settings = ['BearerToken'] # noqa: E501
Expand Down Expand Up @@ -2846,6 +2849,7 @@ def patch_namespaced_custom_object_with_http_info(self, group, version, namespac
all_params.extend(
[
'async_req',
'content_type',
'_return_http_data_only',
'_preload_content',
'_request_timeout'
Expand Down Expand Up @@ -2920,8 +2924,8 @@ def patch_namespaced_custom_object_with_http_info(self, group, version, namespac
['application/json']) # noqa: E501

# HTTP header `Content-Type`
header_params['Content-Type'] = self.api_client.select_header_content_type( # noqa: E501
['application/merge-patch+json']) # noqa: E501
header_params['Content-Type'] = local_var_params.get('content_type', self.api_client.\
select_header_content_type(['application/merge-patch+json'], 'PATCH', body_params))

# Authentication setting
auth_settings = ['BearerToken'] # noqa: E501
Expand Down Expand Up @@ -3024,6 +3028,7 @@ def patch_namespaced_custom_object_scale_with_http_info(self, group, version, na
all_params.extend(
[
'async_req',
'content_type',
'_return_http_data_only',
'_preload_content',
'_request_timeout'
Expand Down Expand Up @@ -3098,8 +3103,8 @@ def patch_namespaced_custom_object_scale_with_http_info(self, group, version, na
['application/json', 'application/yaml', 'application/vnd.kubernetes.protobuf']) # noqa: E501

# HTTP header `Content-Type`
header_params['Content-Type'] = self.api_client.select_header_content_type( # noqa: E501
['application/merge-patch+json']) # noqa: E501
header_params['Content-Type'] = local_var_params.get('content_type', self.api_client.\
select_header_content_type(['application/merge-patch+json'], 'PATCH', body_params))

# Authentication setting
auth_settings = ['BearerToken'] # noqa: E501
Expand Down Expand Up @@ -3202,6 +3207,7 @@ def patch_namespaced_custom_object_status_with_http_info(self, group, version, n
all_params.extend(
[
'async_req',
'content_type',
'_return_http_data_only',
'_preload_content',
'_request_timeout'
Expand Down Expand Up @@ -3276,8 +3282,8 @@ def patch_namespaced_custom_object_status_with_http_info(self, group, version, n
['application/json', 'application/yaml', 'application/vnd.kubernetes.protobuf']) # noqa: E501

# HTTP header `Content-Type`
header_params['Content-Type'] = self.api_client.select_header_content_type( # noqa: E501
['application/merge-patch+json']) # noqa: E501
header_params['Content-Type'] = local_var_params.get('content_type', self.api_client.\
select_header_content_type(['application/merge-patch+json'], 'PATCH', body_params))

# Authentication setting
auth_settings = ['BearerToken'] # noqa: E501
Expand Down
17 changes: 16 additions & 1 deletion kubernetes/client/api_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,7 @@ def select_header_accept(self, accepts):
else:
return ', '.join(accepts)

def select_header_content_type(self, content_types):
def select_header_content_type(self, content_types, method=None, body=None):
"""Returns `Content-Type` based on an array of content_types provided.

:param content_types: List of content-types.
Expand All @@ -505,6 +505,21 @@ def select_header_content_type(self, content_types):

content_types = [x.lower() for x in content_types]

if method and body and method == 'PATCH':
# try to select valid content_type
if 'application/json-patch+json' in content_types and \
'application/merge-patch+json' in content_types:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about changing 'application/merge-patch+json' in content_types: to

('application/merge-patch+json' in content_types or 'application/strategic-merge-patch+json' in content_types):

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. It should be implemented in this way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything related to application/strategic-merge-patch+json will be a patch maintained by us IIUC

if isinstance(body, list):
return 'application/json-patch+json'
else:
# ---
# kubernetes specific - application/strategic-merge-patch+json
# applied as a patch or subclass
if 'application/strategic-merge-patch+json' in content_types:
return 'application/strategic-merge-patch+json'
# ---
return 'application/merge-patch+json'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most native types support both strategic merge patch and JSON merge patch. Just to clarify, if a client wants to send JSON merge patch, it has to use the content_type parameter to hint, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's hard to detect which type of patch has been sent so it's better to expose content_type explicit.


if 'application/json' in content_types or '*/*' in content_types:
return 'application/json'
else:
Expand Down
4 changes: 0 additions & 4 deletions kubernetes/client/rest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,6 @@ def request(self, method, url, query_params=None, headers=None,
if query_params:
url += '?' + urlencode(query_params)
if re.search('json', headers['Content-Type'], re.IGNORECASE):
if headers['Content-Type'] == 'application/json-patch+json':
if not isinstance(body, list):
headers['Content-Type'] = \
'application/strategic-merge-patch+json'
request_body = None
if body is not None:
request_body = json.dumps(body)
Expand Down