-
Notifications
You must be signed in to change notification settings - Fork 219
fix: decouple DependentResource creation from configuration #883
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
metacosm
commented
Jan 28, 2022
- fix: do not call default resource class resolution logic by default
- fix: do not create DependentResources directly from configuration
The idea is to delay instantiation until needed and rely on DependentResourceControllerFactory for this purpose instead. Not sure if we actually need to use an interface there or not.
SonarCloud Quality Gate failed. |
|
||
Class<? extends DependentResource<R, P>> getDependentResourceClass(); | ||
|
||
Class<R> getResourceClass(); |
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 should not be getPrimaryResourceClass()
? or? it's not clear for me.
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.
No, that's actually not the primary resource class but the dependent resource one. The naming is tricky and should probably improved indeed: getDependentResourceClass
returns the DependentResource
implementation class (e.g. DeploymentDependentResource
), while getResourceClass
returns the actual associated resource class (e.g. Deployment
).
Note that I've been trying to be consistent in the parameter types naming for these new classes: R
represents a resource class, while P
represents the primary resource types when it's needed. If we wanted to be more explicit we could use more explicit parameter types (e.g. use Primary
instead of simply P
) but that's not a common pattern in Java code…
|
||
import io.fabric8.kubernetes.api.model.HasMetadata; | ||
|
||
public interface DependentResourceConfiguration<R, P extends HasMetadata> { |
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 are these classes configurable? or what is the point of this configuration?
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.
The goal is to decouple the configuration of the dependent resource from its creation. Previously, this was done by the same class and the AnnotationConfiguration
was doing the instantiating of the DependentResource
implementation which led to it being too complex but also needed to know some details about how the implementation worked (in particular how to deal with Kubernetes dependent resources).
This also forced the quarkus extension to have to instantiate the dependent resource implementation class at build time because it was right there on the ControllerConfiguration.getDependentResources()
method. Well, it's not exactly true because it's possible to delay the instantiation until the method is called but that wasn't clean enough and led to more complications.