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.

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

dynamic/fake not honoring the patch type -> json doc error #478

Closed
vaikas opened this issue Oct 1, 2018 · 6 comments · Fixed by kubernetes/kubernetes#69330
Closed

dynamic/fake not honoring the patch type -> json doc error #478

vaikas opened this issue Oct 1, 2018 · 6 comments · Fixed by kubernetes/kubernetes#69330

Comments

@vaikas
Copy link

vaikas commented Oct 1, 2018

Having a hard time unit testing a Patch operation using a dynamic fake. If I run the code against a real cluster (using the dynamic client, not fake), things work aok, but in unit tests when trying to create a JSONPatch operation it fails always with "invalid JSON document".

It seems to be coming from pkg/util/strategicpatch/patch.go

Looks to me like our use of JSON Patch is not being honored since the patch type is not being used anywhere, and seems like instead it's trying to unserialized into a map[string]interface{} and it fails since JSON patch is an array of patch operations.
https://github.com/kubernetes/client-go/blob/master/dynamic/fake/simple.go#L334

Or, am I holding it somehow wrong and this is supposed to work?
Thanks in advance!

@liggitt
Copy link
Member

liggitt commented Oct 1, 2018

yes, that's a bug. the patch type should definitely be included in the fake patch action, and the case PatchActionImpl should switch on the known patch types.

@vaikas
Copy link
Author

vaikas commented Oct 2, 2018

I can take a look at implementing this.

@deads2k
Copy link
Contributor

deads2k commented Oct 2, 2018

I can take a look at implementing this.

Thanks. @p0lyn0mial you've been in the dynamic client. Are you up for a review?

@vaikas
Copy link
Author

vaikas commented Oct 2, 2018

FYI, I was struggling a bit getting docker set up so I could run the hack update deps, but there are still breakages, but just curious about the general direction while I work on the breakages.

@p0lyn0mial
Copy link
Contributor

@p0lyn0mial you've been in the dynamic client. Are you up for a review?

yes :)

@vaikas
Copy link
Author

vaikas commented Oct 3, 2018

So, it seems like just running the hack script as per here is not enough:
https://github.com/kubernetes/community/blob/master/contributors/devel/godep.md

Trying to figure out what else needs to change...

"Saving deps in staging repos
Kubernetes stores some code in a directory called staging which is handled specially, and is not covered by the above. If you modified any code under staging, or if you changed a dependency of code under staging (even transitively), you'll also need to update deps there:

./hack/update-staging-godeps.sh
Then commit the changes generated by the above script."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants