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

improve: remove seamless type change #2726

Closed
wants to merge 2 commits into from
Closed

Conversation

csviri
Copy link
Collaborator

@csviri csviri commented Mar 9, 2025

The curret implementation changed the underlying type, what is not we expect when passing an instance
will create issues when some compares GVKs that are first passed then read.
On the other hand if the user passes an instance of GVKP just can cast back.

Signed-off-by: Attila Mészáros [email protected]

The curret implementation changed the underlying type, what is not we expect when passing an instance
will create issues when some compares GVKs that are first passed then read.
On the other hand if the user passes an instance of GVKP just can casr back.

Signed-off-by: Attila Mészáros <[email protected]>
@openshift-ci openshift-ci bot requested review from metacosm and xstefank March 9, 2025 20:13
Signed-off-by: Attila Mészáros <[email protected]>
Copy link
Collaborator

@xstefank xstefank left a comment

Choose a reason for hiding this comment

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

Commenting, since I think that I miss some rules. Yes, user can fix this be downcating but this will be a breaking change. So it this is OK to push non-major release?

@csviri csviri changed the title improve: remove sealess type change improve: remove seamless type change Mar 10, 2025
@csviri
Copy link
Collaborator Author

csviri commented Mar 10, 2025

Commenting, since I think that I miss some rules. Yes, user can fix this be downcating but this will be a breaking change. So it this is OK to push non-major release?

Usually we discuss such breaking changes based on some possible impact, the Generic Kubernetes Resource APIs is already something quite limited, usually used to just cover resources for which there is no generated Java classes - what is rare - since there is a generator, or you doing some dynamic resource handling. This was asked also from some users, but probably the major use case is the Glue operator, where this is causing problem during the update. (Also I see usage some places in QOSDK - but not for this API). Is this used in QOSDK or planned, @metacosm @xstefank ?

@xstefank
Copy link
Collaborator

@csviri not to my knowledge. My question was purely based on semantic versionsing.

@metacosm
Copy link
Collaborator

I don't understand why this change is needed. As you can pass a GVKP to create a GenericKubernetesDependentResource is perfectly normal to use the wider type. Also, as pointed out, this would be a breaking change.

@csviri
Copy link
Collaborator Author

csviri commented Mar 10, 2025

I don't understand why this change is needed. As you can pass a GVKP to create a GenericKubernetesDependentResource is perfectly normal to use the wider type. Also, as pointed out, this would be a breaking change.

I literally was debugging 3 hours because of this, since you pass GVK and it under the hood changes it to GVKP, where the equal fails from that point. So we either do this this way or override GVK / GVKP equals method so they are aware of each other:

https://github.com/java-operator-sdk/kubernetes-glue-operator/blob/f1562a12f4254d30e6e80bcd7950779985d6c3df/src/main/java/io/javaoperatorsdk/operator/glue/reconciler/glue/GlueReconciler.java#L376-L383

But I prefer this since it can be very confusing for other users also.

@csviri
Copy link
Collaborator Author

csviri commented Mar 10, 2025

@metacosm where is this used now at all? I mean GVKP.

@csviri
Copy link
Collaborator Author

csviri commented Mar 10, 2025

Hmm, so the problem is that if I pass GVK it transforms it seamlessly to GVKP, so when I get it back that is not comparable to the input.

So we have a history with this also that GVKP is not used in JOSDK at all, we added this because of RBAC feature of in QOSDK, but it is not used atm neither in QOSDK. Is there still the intention to use it? Shouldn't we just move GVKP to QOSDK in that case? Or remove it in general, at least in scope of JOSDK this does not make sense to keep.

@metacosm
Copy link
Collaborator

I think the appropriate fix should be #2731. It needs to stay in JOSDK because downstream projects do not control how these objects are created. Generally speaking, plurals are pretty badly handled in Kubernetes despite the fact that they are quite central…

@csviri
Copy link
Collaborator Author

csviri commented Mar 12, 2025

replaced by: #2731

@csviri csviri closed this Mar 12, 2025
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 this pull request may close these issues.

3 participants