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

Patching of a Custom Resource does not work in v6.1.1 #4441

Closed
danpfe opened this issue Sep 22, 2022 · 8 comments · Fixed by #4446
Closed

Patching of a Custom Resource does not work in v6.1.1 #4441

danpfe opened this issue Sep 22, 2022 · 8 comments · Fixed by #4446
Assignees
Labels
Milestone

Comments

@danpfe
Copy link

danpfe commented Sep 22, 2022

Have a custom resource, defined as:

@Group("mysamplegroup")
@Version("v1")
public class Sample extends CustomResource<SampleSpec, SampleStatus> implements Namespaced {
 // ...
}

When I do client.resource(some_Default_Resource_Such_As_A_Pod).patch(); it works just fine, doing the same on the above custom resource simply does nothing.

Doing it the old and deprecated way client.customResource(Sample.class).inNamespace("default").withName("something").patch(myCustomResource); works, however.

Is this a bug or am I misunderstanding something about the new API?

Thanks!

@rohanKanojia
Copy link
Member

Looks like a bug to me, somehow it seems to be sending same object as base and updated ones and generating incorrect diff.

@danpfe
Copy link
Author

danpfe commented Sep 22, 2022

Don't know if that helps but here is the "sample-operator" that I updated to 6.1.1 and that I used as template to build other operators (that worked with the legacy API): https://github.com/codelair-io/example-k8s-operator-java/tree/master/src/main/java/io/codelair/examples/kubernetesoperator

@shawkins
Copy link
Contributor

it works just fine, doing the same on the above custom resource simply does nothing.

@danpfe @rohanKanojia the logic is the same either way. I think there's a problem no matter which resource type you use.

This is a regression with #4142 I think the intent of the change was so that patch(item) methods would be against the current item, rather than what was from the server - resource(item1).patch(item2) will create a diff between item1 and item2, which is also consistent with how the edit and accept methods worked. With the patch() call it's just diffing to itself.

I think that it's better to have the patch() method specifically update the context to ensure the server side item is used, rather than going back to always loading the server side item as the base.

@manusa manusa added the bug label Sep 23, 2022
@manusa
Copy link
Member

manusa commented Sep 23, 2022

I think that it's better to have the patch() method specifically update the context to ensure the server side item is used, rather than going back to always loading the server side item as the base.

I'm not sure if this would apply to all patch strategies.

@danpfe
Copy link
Author

danpfe commented Sep 23, 2022

it works just fine, doing the same on the above custom resource simply does nothing.

@danpfe @rohanKanojia the logic is the same either way. I think there's a problem no matter which resource type you use.

Hi @shawkins. I can retest this. I have a private operator that I can't share here where I believe patch() worked on another resource but maybe I just confused it with other modifier-methods.

@shawkins
Copy link
Contributor

I'm not sure if this would apply to all patch strategies.

@manusa this issue only affects JSON patches when there is a context item and no item passed in - that's only the patch() or the patch(context) methods.

shawkins added a commit to shawkins/kubernetes-client that referenced this issue Sep 23, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Sep 23, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Sep 23, 2022
@danpfe
Copy link
Author

danpfe commented Sep 26, 2022

it works just fine, doing the same on the above custom resource simply does nothing.

@danpfe @rohanKanojia the logic is the same either way. I think there's a problem no matter which resource type you use.

Hi @shawkins. I can retest this. I have a private operator that I can't share here where I believe patch() worked on another resource but maybe I just confused it with other modifier-methods.

So I checked, you're right. patch() doesn't work no matter the resource, patchStatus() seems to works though :)

@shawkins
Copy link
Contributor

patchStatus() seems to works though

Yes that is a json merge, not a json patch, so no diff is needed.

@manusa manusa added this to the 6.2.0 milestone Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants