-
Notifications
You must be signed in to change notification settings - Fork 219
fix: retrieve DependentResource based on name instead of class #1058
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
Conversation
Could the name be provided by the
|
And name could be latter override by configuration on the annotation if needed |
Looks good so far, my question is also that, the name should be mandatory in the annotation? Or how do you see that? |
If not provided, then it should take the type canonical name and raise an error if there is a dup |
Yep, probably a sensible default makes sense in this case. |
Yes, that's the plan indeed.
I think so, yes. |
Yes, that is the plan but this PR is not finished yet. Great to see some early feedback that validates what I was thinking, though! 😄 |
75c1a4b
to
eb4e720
Compare
01015f0
to
cc9bae2
Compare
cc9bae2
to
12b9e64
Compare
bb073e7
to
f1149fa
Compare
...vaoperatorsdk/operator/api/reconciler/dependent/managed/ManagedDependentResourceContext.java
Outdated
Show resolved
Hide resolved
Still having issues with this, so switching back to draft mode as I uncovered issues that need to be fixed with the event source registration. Basically, the dependent resource name should be propagated to the associated event source if it exists. The MySQL sample is also behaving weirdly… |
22ccb38
to
14c2f56
Compare
...vaoperatorsdk/operator/api/reconciler/dependent/managed/ManagedDependentResourceContext.java
Outdated
Show resolved
Hide resolved
7f82f54
to
f4be7ef
Compare
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java
Outdated
Show resolved
Hide resolved
* | ||
* @return the resource type associated with this DependentResource | ||
*/ | ||
Class<R> resourceType(); |
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.
Why we need this at this level?
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.
To make things explicit so that we don't need to rely on a default implementation that will not work in all situations.
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.
I mean are we sure that this will be needed in all cases?
I mean I can imaging a dependent resource implementation that does not need resource type.
I thought it is something that is required for quarkus.
I guess we can then delete ResourceTypeAware
.
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.
Yeah, I'm conflicted on this… but the problem is that I haven't had time to spend completely focused on the extension so I'm not 100% sure about it.
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java
Show resolved
Hide resolved
Can we merge this? |
This causes issues if the class is proxied for any reason (e.g. because it's a CDI bean).
ca0f38e
to
5cefd44
Compare
Kudos, SonarCloud Quality Gate passed! |
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.
sure, LGTM!
This causes issues if the class is proxied for any reason (e.g. because
it's a CDI bean).