-
Notifications
You must be signed in to change notification settings - Fork 220
Extract interfaces for optional behavior of DependentResource #949
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
Feb 16, 2022
- feat: extract EventSourceProvider interface to clean up common use case
- feat: extract DependentResourceConfigurator interface
- fix: provide configuration via interface on reconciler
Operator.register(Reconciler, ControllerConfigurationOverrider) actually relied on non-documented and non-standard behavior of the default ConfigurationService implementation, which automatically creates the configuration associated with the reconciler if it doesn't exist.
*/ | ||
public <R extends HasMetadata> void register(Reconciler<R> reconciler, | ||
Consumer<ControllerConfigurationOverrider> configOverrider) { | ||
final var controllerConfiguration = configurationService.getConfigurationFor(reconciler); |
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 relied on the fact that the default ConfigurationService
implementation will automatically create the configuration associated with the reconciler if it doesn't exist. However, in the general case, this would actually throw an NPE as this would return null
: https://github.com/java-operator-sdk/java-operator-sdk/blob/323372a2e7b5b05f25a3af1162b420d5d95c19e5/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractConfigurationService.java#L54-L62
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 default is to add AnnotationControllerConfiguration, see:
http://github.com/java-operator-sdk/java-operator-sdk/blob/dbf6c2e001ff5bba701f231babc9affc8c7433a7/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/DefaultConfigurationService.java#L33-L33
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.
Like the other changes, but have basically conceptual problems with this, using DependentResourceConfigurationProvider
makes the whole Annotation based Dependent Resource definition kinda loosing it's meaning.
So from now in the background what will happen is just the DependentResource will be instantiated (with constaint of empty constructor present), and the the configuration that is explicitly implemented in the reconciler is passed to it.
I really liked that the configuration of the dependent resources is decoupled from the reconciler, since it makes then the reconciler much cleaner. Now rather I would then do just standalone DependentResources instead of this, when implementing an operator. Since we loose even that decoupling.
(And of course the reconcile and prepare event sources will be called explicitly in the background but that is again not a huge benefit and rather could be confusing / hard to understand for people who start with. So has negative sides too)
So I think we should either:
- Fix the configuraiton with using ControllerConfiguration mechanism
- Or just drop the whole annotation based DependentResource definitions (/managed dependent resources) since it is starts loosing it's benefit, and seems to be too hard and even confusing to implement / work with it.
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 default is to add AnnotationControllerConfiguration, see: http://github.com/java-operator-sdk/java-operator-sdk/blob/dbf6c2e001ff5bba701f231babc9affc8c7433a7/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/DefaultConfigurationService.java#L33-L33
checked, this works no on next
branch.
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.
An alternative what a I see is to pass this on registration:
operator.register(controller, cofiguration, dependentResourceConfiguration)
and
operator.register(controller, dependentResourceConfiguration)
So decouple it from controller configuration.
import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext; | ||
import io.javaoperatorsdk.operator.processing.event.source.EventSource; | ||
|
||
public interface EventSourceProvider<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.
+1
@@ -0,0 +1,5 @@ | |||
package io.javaoperatorsdk.operator.api.reconciler.dependent; | |||
|
|||
public interface DependentResourceConfigurator<C> { |
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.
+1
SonarCloud Quality Gate failed. |
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.
LGTM