Skip to content

Fix for patch_* to work #98

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.

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

Merged
merged 1 commit into from
Jan 21, 2017
Merged

Conversation

dims
Copy link
Collaborator

@dims dims commented Jan 14, 2017

Hack to get patch_namespaced_config_map and patch_namespaced_service
etc to work by essentially switching over the content-type from:
application/json-patch+json
to:
application/strategic-merge-patch+json

Fixes #93

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 14, 2017
@codecov-io
Copy link

codecov-io commented Jan 14, 2017

Current coverage is 94.52% (diff: 100%)

Merging #98 into master will not change coverage

@@             master        #98   diff @@
==========================================
  Files             9          9          
  Lines           658        658          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits            622        622          
  Misses           36         36          
  Partials          0          0          

Powered by Codecov. Last update 4c3aced...9c6c965

@mbohlool
Copy link
Contributor

That would definitely fix #93. From the description, it look like we just change the content type (and if that is the case, I would rather do it in preprocessing step on the spec instead of in the code), but we also deal with the fact that for those type of APIs, we get an empty model and the model does not have anything in it, thus make it impossible to pass data. I would suggest we create a special model for these cases (or even better, edit their models) that accepts json and do the right thing to pass that json to the api call instead of hacking the client, what do you think?

@dims
Copy link
Collaborator Author

dims commented Jan 17, 2017

yes, this was a straight port of hack from what we had in python-k8sclient. yes, we can do slightly better as you suggest.

@mbohlool
Copy link
Contributor

Can we at least have an issue and a TODO attached to that code pointing to that issue? (also this branch has conflicts need to be fixed)

@mbohlool mbohlool self-assigned this Jan 18, 2017
@dims
Copy link
Collaborator Author

dims commented Jan 18, 2017

@mbohlool i'll iterate on this PR and ping you back when ready. thanks.

@dims dims force-pushed the fix-patch-tests branch 7 times, most recently from 37ec2df to 432ce71 Compare January 18, 2017 18:57
@mbohlool
Copy link
Contributor

Oh, cool, I was working on side for a fix for this and intstr both but look like you are working on it. You need to edit client-update.sh to keep your changes. Let me know if I can be of any help.

@dims
Copy link
Collaborator Author

dims commented Jan 18, 2017

@mbohlool : ouch. sorry. i've updated the client-update.sh as well (simple git checkout seems to be enough).

@mbohlool
Copy link
Contributor

That will work, however if you actually had changed that file, it will undo your changes too. Don't worry about that though, I have the changed script and can send a PR for that. Keep the git checkout for now.

@mbohlool
Copy link
Contributor

and let me know when this is ready for review.

@dims
Copy link
Collaborator Author

dims commented Jan 19, 2017

@mbohlool this is ready for review now. thanks

@mbohlool
Copy link
Contributor

I thought the idea of adding a model for the patch is to not hack the rest client, but I still see the hack in. My suggestion is to add a custom json serializing method (name to be decided) to the model and in the rest client serializing (sanitize_for_serialization), check for that method and use it if exists, to serialize the model. This way you shouldn't need any hack.

@dims
Copy link
Collaborator Author

dims commented Jan 20, 2017

@mbohlool : ok i think i misunderstood. If you want to take a whack at adding a custom json serializing method, that would be cool. thanks. please feel free to reuse any of the code/test in this PR. (or even modifying this PR).

Thanks,
Dims

@mbohlool
Copy link
Contributor

Sure, I can do that. Lets just merge the first commit of this PR (the one with the hack), can you please remove the rest of the commits? (probably push them to another branch on your repo so we can refer to them later).

@mbohlool
Copy link
Contributor

I don't think I can modify this PR as it is coming form your repo. Please keep the first commit and rebase and we can merge this. Thanks.

@dims
Copy link
Collaborator Author

dims commented Jan 20, 2017

Done. thanks @mbohlool

@mbohlool
Copy link
Contributor

@dims The PR needs rebase.

Hack/workaround to get patch_namespaced_config_map and patch_namespaced_service
etc to work by essentially switching over the content-type from:
application/json-patch+json
to:
application/strategic-merge-patch+json

Fixes kubernetes-client#93
@dims
Copy link
Collaborator Author

dims commented Jan 21, 2017

@mbohlool oops done.

@mbohlool mbohlool merged commit a3e922a into kubernetes-client:master Jan 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants