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

Client.resource(resource).get() does not return latest resource #4574

Closed
gyfora opened this issue Nov 10, 2022 · 10 comments · Fixed by #4601
Closed

Client.resource(resource).get() does not return latest resource #4574

gyfora opened this issue Nov 10, 2022 · 10 comments · Fixed by #4601
Assignees
Milestone

Comments

@gyfora
Copy link

gyfora commented Nov 10, 2022

Describe the bug

I have been trying to use the following code snippet to get the latest version of a given custom resource:

var latest = client.resource(resource).get();

However it seems that this does not actually return the latest version but simply the resource I passed to the call...

On the contrary when I do:

var latest = client.resources(MyCR.class, MyCRList.class)
                                    .inNamespace(ns)
                                    .withName(name)
                                    .get();

it works and gives the latest resource.

This seems to be an inconsitency in the api.

Fabric8 Kubernetes Client version

6.2.0

Steps to reproduce

Compare the output of:

var latest = client.resource(resource).get();

vs

var latest = client.resources(MyCR.class, MyCRList.class)
                                    .inNamespace(ns)
                                    .withName(name)
                                    .get();

Expected behavior

They should both return the latest version of the resource. But only the second works

Runtime

minikube

Kubernetes API Server version

1.25.3@latest

Environment

macOS

Fabric8 Kubernetes Client Logs

No response

Additional context

No response

@shawkins
Copy link
Contributor

This seems to be an inconsitency in the api.

You need to use fromServer() to force the fetch for a get when using resource(foo). When the docs refer to the "current item" that does include the one passed into the resource method.

@gyfora
Copy link
Author

gyfora commented Nov 10, 2022

Maybe I just don't understand the design here but I find it extremely odd that a call to a client with a get returns the exact object that you passed to it.

I am sure many people will intuitively misunderstand this

@gyfora
Copy link
Author

gyfora commented Nov 10, 2022

I have tried to migrate to the new .resource(instance)... style api from the older deprecated methods but I just realized I have misued it in the similar way.

Do you think it makes sense to change the default behaviour to help users? I think if they just want to get the same resource back they would not call the client.

@shawkins
Copy link
Contributor

Maybe I just don't understand the design here but I find it extremely odd that a call to a client with a get returns the exact object that you passed to it.

I'm not sure what the rationale was for having this naming / functioning. Feel free to capture something if want to propose different signatures.

I am sure many people will intuitively misunderstand this

Sure, feel free to open a pr for any missing javadocs.

@shawkins
Copy link
Contributor

Do you think it makes sense to change the default behaviour to help users? I think if they just want to get the same resource back they would not call the client.

This behavior was already how the client functioned for the existing resource and resourceList methods, so it carried forward once the resource method was generalized - as such existing usage wouldn't change. I don't think it was spelled out in the release notes that you need to call fromServer when switching to the resource api from the other style - that was an over site.

As for changing the default behavior of what a get means that can be expanded to @manusa @rohanKanojia and others.

@manusa
Copy link
Member

manusa commented Nov 11, 2022

As for changing the default behavior of what a get means that can be expanded to...

Considering #4533 this might be a problem, since users might actually want to use kubernetesClient.load(InputStream), kubernetesClient.resource(InputStream), and others to deserialize the streams.

However, I do agree that the current default behavior is misleading and is error-prone. So I think it does make sense to change the default behavior (always fromServer), but as long as we provide a way to retrieve the current local item(s). Adding relevant documentation and breaking change notice would also be a must if we opt for this.

@gyfora
Copy link
Author

gyfora commented Nov 11, 2022

It feels like if the user calls

kubernetesClient.load(InputStream).get()

and expect to get the deserialized value, then we are a little bit mixing two very different functionalities.

One is a simple deserialization problem (why not just use the Jackson ObjectMapper?) and one is the kubernetes client getting the resource which as you point out should always get fromServer to be consistent.

@manusa
Copy link
Member

manusa commented Nov 11, 2022

and expect to get the deserialized value, then we are a little bit mixing two very different functionalities....

Yep, but the functionality is there (just like the fromServer() modifier) since the beginning and some users might be relying on it.

As I said, I'm not against changing the default behavior, especially because the semantics of the resource().get() are more unclear than load().fromServer().get. But we should deprecate fromServer and provide a clear notice of the behavioral change.

@shawkins
Copy link
Contributor

But we should deprecate fromServer and provide a clear notice of the behavioral change.

@manusa to make sure the set of changes being proposed:

  • make / document a breaking change of get() as always getting latest from the server

  • deprecate fromServer (which would now be a no-op), and add an item() method instead that returns the current item if one is associated (that is the Resource was created from resource, resourceList, or load).

There will be some associated test and internal changes to align to this. Let me know if this sounds right and I can make the changes.

@manusa
Copy link
Member

manusa commented Nov 22, 2022

This sounds good (sorry for the belated reply)

shawkins added a commit to shawkins/kubernetes-client that referenced this issue Nov 24, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Nov 24, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Nov 24, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Nov 24, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Nov 28, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Nov 28, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Nov 28, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Nov 29, 2022
@manusa manusa added this to the 6.4.0 milestone Dec 12, 2022
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.

3 participants