-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
package io.javaoperatorsdk.operator.api.config; | ||
|
||
import io.fabric8.kubernetes.api.model.HasMetadata; | ||
|
||
public interface DependentResourceConfiguration<R, P extends HasMetadata> { | ||
|
||
Class<? extends DependentResource<R, P>> getDependentResourceClass(); | ||
|
||
Class<R> getResourceClass(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should not be There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,26 @@ | ||
package io.javaoperatorsdk.operator.api.reconciler.dependent; | ||
|
||
import java.lang.reflect.InvocationTargetException; | ||
|
||
import io.fabric8.kubernetes.api.model.HasMetadata; | ||
import io.javaoperatorsdk.operator.api.config.DependentResource; | ||
import io.javaoperatorsdk.operator.api.config.DependentResourceConfiguration; | ||
|
||
public interface DependentResourceControllerFactory<P extends HasMetadata> { | ||
|
||
default <R> DependentResourceController<R, P> from(DependentResource<R, P> dependent) { | ||
// todo: this needs to be cleaned-up / redesigned | ||
return dependent instanceof DependentResourceController | ||
? (DependentResourceController<R, P>) dependent | ||
: new DependentResourceController<>(dependent); | ||
default <T extends DependentResourceController<R, P, C>, R, C extends DependentResourceConfiguration<R, P>> T from( | ||
C config) { | ||
try { | ||
final var dependentResource = config.getDependentResourceClass().getConstructor() | ||
.newInstance(); | ||
if (config instanceof KubernetesDependentResourceConfiguration) { | ||
return (T) new KubernetesDependentResourceController(dependentResource, | ||
(KubernetesDependentResourceConfiguration) config); | ||
} else { | ||
return (T) new DependentResourceController(dependentResource, config); | ||
} | ||
} catch (NoSuchMethodException | InvocationTargetException | InstantiationException | ||
| IllegalAccessException e) { | ||
throw new IllegalArgumentException(e); | ||
} | ||
} | ||
} |
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 theDependentResource
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.