feat: configure resource class and name for controller #1781
Conversation
| } | ||
|
|
||
| public ControllerConfigurationOverrider<R> withResourceClass(Class<R> resourceClass) { | ||
| this.resourceClass = resourceClass; |
There was a problem hiding this comment.
It feels weird to override a given controller configuration with a different resource type so I'm not sure about this one…
There was a problem hiding this comment.
I think it was added there if there is some hierarchy or something in reconciler, and cannot be detected as we do now. in that terms it makes sense to me.
There was a problem hiding this comment.
The problem is that it opens the door for improper usage so I'd rather see this support moved somewhere outside of the overrider. The goal of the overrider is to change the configuration provided by the annotations at runtime, not to create a new configuration from scratch. As such, since the resource type is mandated by the parameter type of the Reconciler implementation, it shouldn't be possible to override it at runtime so the more I think about it, the more against this change I am.
There was a problem hiding this comment.
Hmm, that is right. The problem is it's not possible to specify it by @ControllerConfiguration. Shall we put it rather there?
Would be a better solution.
There was a problem hiding this comment.
That would be a huge change and a redundant information with the Reconciler implementation.
There was a problem hiding this comment.
Again, I fail to see how things would work if you were to override the resource type and end up with something that doesn't match your Reconciler implementation… If people want to have a hierarchy, the resource type parameter should be propagated along it.
There was a problem hiding this comment.
kk, let's remove this option
There was a problem hiding this comment.
revisited this again, and we would break this functionality provided by the users:
https://github.com/java-operator-sdk/operator-framework-spring-boot-starter/blob/182b1111990f99d6516d2965f54dddca8c1cf6de/starter/src/main/java/io/javaoperatorsdk/operator/springboot/starter/OperatorAutoConfiguration.java#L103-L107
if we remove this. What is bad.
There was a problem hiding this comment.
Basically there is no other way to configure this through our public API, just maybe hacking it with some custom config service, what is problematic. So I think we should allow this to be configured and document it, that this is just for framework support.
There was a problem hiding this comment.
Basically there is no other way to configure this through our public API, just maybe hacking it with some custom config service, what is problematic. So I think we should allow this to be configured and document it, that this is just for framework support.
That is not true: the current way to do it is to provide your own ControllerConfiguration implementation (or extend the existing ConfigurationService implementations) but I do agree that we could make things easier.
csviri
force-pushed
the
override-class-controller
branch
from
e7affe1 to
fa57cd7
Compare
|
Note that this is technically a breaking API change. |
| return (Class<P>) Utils.getFirstTypeArgumentFromSuperClassOrInterface(getClass(), | ||
| ControllerConfiguration.class); | ||
| } | ||
| Class<P> getResourceClass(); |
There was a problem hiding this comment.
We should keep a default implementation to avoid breaking the API.
There was a problem hiding this comment.
Well, yes, althoug not really the API, it's the implementation. But we should remove it in the future, to make it more explicit that this is not used
There was a problem hiding this comment.
It is ok to do ti for next? (I mean a separate PR, wdyt?)
There was a problem hiding this comment.
No, this method should stay as it's needed by some implementations, Quarkus in particular. And it is breaking the API because implementations that were relying on the provided default implementation won't compile anymore with this change.
There was a problem hiding this comment.
reverted and commented it for now, but I guess we can adjust also quarkus in the future if it will be removed.
For now fine to have it there IMO.
This PR should be ready now, also the adjusted spring boot part.
There was a problem hiding this comment.
I don't think that this will change in Quarkus because the resource class is resolved at build time and is therefore fixed then, not resolved at runtime, which is why we need this method to stay as-is.
|
Kudos, SonarCloud Quality Gate passed! |
this is mostly needed so we are able to nicely upgrade the spring boot starter to 4.2.x of JOSDK