-
Notifications
You must be signed in to change notification settings - Fork 23
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
feature and refactor: add lombok, introduce package structure and add interface to configure dependent resources #208
base: main
Are you sure you want to change the base?
Conversation
… interface to configure dependent resources (#1) * refactor: add lombok * feature: add interface to configure dependent resources
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.
Overall looks good to me, although it brings some breaking changes, not sure how much it would be affecting users, probably not that much. So we can release this as part of a minor version IMO
@UtilityClass | ||
public class ReconcilerRegistrationUtil { | ||
|
||
public List<DependentResourceConfigurator> filterConfigurators(Reconciler<?> 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.
It might deserve a unit test, since logic is not trivial.
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.
good point, thank you. added unit tests
@FunctionalInterface | ||
public interface DependentResourceConfigurator { | ||
|
||
String getName(); |
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.
Just one note regarding to this, altevatively we could avoid inheritance, so the configuration bean would not inherit this interface, but we could put here also method getConfiguraiton
that would result in the configuration bean.
But no strong opinion one way or another.
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.
or as an atlernative we could have this just as an annotation on top of a conig bean: @DependentConfiguration(dependentName="dr name", reconcilerName="optional reconciler name")
Note: lombok might be a matter of further discussions, but I'm fine also to add it in this PR, just might be removed in the subsequent ones. |
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
Hi @1nval1d , do you still plan to work on this (it is a draft) or we can merge it? |
import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResourceConfig; | ||
import io.javaoperatorsdk.operator.springboot.starter.DependentResourceConfigurator; | ||
|
||
public class ConfigMapDRConfigurator extends KubernetesDependentResourceConfig<ConfigMap> |
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.
nit: maybe name it ConfigMapDRConfig
@1nval1d are still planning to work on this PR? in case we can merge it to |
BREAKING CHANGE:
Reactoring - introducing new package structure:
basePath: io.javaoperatorsdk.operator.springboot.starter
Introduced new interface to configure dependent resources
Usage: create a bean , that implements the DependentResourceConfigurator interface, return the name of the dependent resource in the getName method. Expect the bean in the configureWith-method of your dependent resource. Make sure to implement other required interfaces.