-
Notifications
You must be signed in to change notification settings - Fork 218
feat: automatically derive the the dependent resource type if not specified #2772
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine, it just only works with flat class hierarchy.
One ask, could you pls show what error message we get if the hierarchy is not flat? and this does not work.
I think we had something similar to this at some point but decided to make it explicit because there were too many cases where the automatic resolution of the type was causing issues (iirc). Having a no-arg constructor might induce people to initialize their dependent resources with it and then get weird exceptions when the automatic resolution fails. |
d1db8d5
to
7b69822
Compare
…cified Signed-off-by: xstefank <[email protected]>
I changed the logic so it now finds the first generic type in the hierarchy and takes the first parameter. We can take it even further and check that we hit some class for JOSDK but I believe this is enough and it allows us to create a test - see https://github.com/operator-framework/java-operator-sdk/pull/2772/files#diff-f5452835ea6fba16e03d5593e7efdcb2497f78368e84b1aeebf3682a6e5953bc. I agree that this will not cover all use cases but we are not removing the constructor that takes the type so if any user runs into issue with the default derivation of the resource type, they can still override it with anything they want. BTW Chris, this todo inspired me to take a look at this :) https://github.com/quarkiverse/quarkus-operator-sdk/blob/main/samples/exposedapp/src/main/java/io/halkyon/DeploymentDependent.java#L20 |
@@ -116,4 +126,14 @@ protected void onUpdated(P primary, R updated, R actual, Context<P> context) { | |||
private RecentOperationCacheFiller<R> recentOperationCacheFiller() { | |||
return (RecentOperationCacheFiller<R>) eventSource; | |||
} | |||
|
|||
@SuppressWarnings("unchecked") | |||
private Class<R> findFirstGenericSuperclass( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should throw a meaningful exception when we cannot find the class, could pls add that, also show it works in a unit test?
Just changing the samples for now. Let me know what you think and I can change all resources or even deprecate some other ctors.