WIP: unify configuration #1440

metacosm · 2022-09-02T17:27:58Z

refactor: cleaner separation between instantiation and configuration
refactor: move instantiation functionality to Utils
refactor: unify context information creation, move it to Utils
refactor: improve getFirstTypeArgumentFromInterface
feat: introduce AnnotationDependentResourceConfigurator concept

The idea is to be able to configure dependents using annotations directly so that we can remove the special handling of KubernetesDependentResource from AnnotationControllerConfiguration. This results in dependent resources being instantiated and configured directly when processed from the annotations in the managed case, thus rendering the DependentResourceFactory concept obsolete. This should also lead to further simplification later.

sonarqubecloud · 2022-09-05T07:59:16Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
12 Code Smells

67.8% Coverage
0.0% Duplication

csviri

Overall about the dynamic configuration and unification, what is a very good idea IMO, just few points / tech details (other than the comments):

The DependentResourceConfigurator does not seems to be used anywhere directly, it's always at the end it's subinterface: AnnotationDependentResourceConfigurator
What AnnotationDependentResourceConfigurator does it basically two responsibilities, creates the configuration from from annotation, and marks that which annotation is related to that configuration.
Could we decouple this? Like having DependentResourceConfigurator interface, that receives the configuration. It does not need to be annotation it can go just from an override or anything. But would separate the creation maybe of the confoguration from annotation. One way would be maybe with a nice(?) is to put an new annotation for the configuration annotation, that says how to produce a config from this annotation. Like for @SchemaConfig:

@ConfigInput(configFactory = AnnotationSchemaConfigFactory.class)
@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.TYPE})
public @interface SchemaConfig {
 
  int pollPeriod() default DEFAULT_POLL_PERIOD;

 // omitted 
}

It will create the configuration instance from this annotation, calling a method on the factory: AnnotationSchemaConfigFactory

csviri · 2022-09-06T08:24:09Z

...ramework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java

@@ -140,8 +140,9 @@ default ObjectMapper getObjectMapper() {
    return Serialization.jsonMapper();
  }

+  @Deprecated(forRemoval = true)
  default DependentResourceFactory dependentResourceFactory() {


Not sure if understand this. It's null since not used anymore I guess.

Backwards compatibility :)

csviri · 2022-09-06T08:58:13Z

...javaoperatorsdk/operator/api/reconciler/dependent/managed/DependentResourceConfigurator.java

@@ -1,5 +1,9 @@
 package io.javaoperatorsdk.operator.api.reconciler.dependent.managed;

+import java.util.Optional;
+
 public interface DependentResourceConfigurator<C> {


I don't really get this interface, it both can receive and produce a configuration? Its because AnnotationDependentResourceConfigurator needs to be supported to?

Backwards compatibility.

csviri · 2022-09-06T09:05:15Z

...hema/src/main/java/io/javaoperatorsdk/operator/sample/dependent/SchemaDependentResource.java

@@ -24,13 +26,18 @@
 import static io.javaoperatorsdk.operator.sample.dependent.SecretDependentResource.MYSQL_SECRET_USERNAME;
 import static java.lang.String.format;

+@SchemaConfig(pollPeriod = 700, host = "127.0.0.1",
+    port = SchemaDependentResource.LOCAL_PORT,
+    user = "root", password = "password") // NOSONAR: password is only used locally, example only
 public class SchemaDependentResource
    extends PerResourcePollingDependentResource<Schema, MySQLSchema>
    implements EventSourceProvider<MySQLSchema>,
    DependentResourceConfigurator<ResourcePollerConfig>,


I guess this can be removed, it's enough just to have AnnotationDependentResourceConfigurator ?

It is, but I wanted to make things backwards-compatible. Removing that constraint would simplify several things, actually.

csviri · 2022-09-06T09:05:49Z

...hema/src/main/java/io/javaoperatorsdk/operator/sample/dependent/SchemaDependentResource.java

@@ -24,13 +26,18 @@
 import static io.javaoperatorsdk.operator.sample.dependent.SecretDependentResource.MYSQL_SECRET_USERNAME;
 import static java.lang.String.format;

+@SchemaConfig(pollPeriod = 700, host = "127.0.0.1",
+    port = SchemaDependentResource.LOCAL_PORT,
+    user = "root", password = "password") // NOSONAR: password is only used locally, example only


I guess the password here should have some placeholder value for "not set"

Yes, this isn't a realistic implementation, just showing here how it is possible to also use annotations to configure dependents.

metacosm · 2022-09-06T15:41:40Z

Overall about the dynamic configuration and unification, what is a very good idea IMO, just few points / tech details (other than the comments):

The DependentResourceConfigurator does not seems to be used anywhere directly, it's always at the end it's subinterface: AnnotationDependentResourceConfigurator

It was kept for backwards compatibility.

What AnnotationDependentResourceConfigurator does it basically two responsibilities, creates the configuration from from annotation, and marks that which annotation is related to that configuration.
Could we decouple this? Like having DependentResourceConfigurator interface, that receives the configuration. It does not need to be annotation it can go just from an override or anything. But would separate the creation maybe of the confoguration from annotation. One way would be maybe with a nice(?) is to put an new annotation for the configuration annotation, that says how to produce a config from this annotation. Like for @SchemaConfig:
@ConfigInput(configFactory = AnnotationSchemaConfigFactory.class)
@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.TYPE})
public @interface SchemaConfig {
 
  int pollPeriod() default DEFAULT_POLL_PERIOD;

 // omitted 
}
It will create the configuration instance from this annotation, calling a method on the factory: AnnotationSchemaConfigFactory

That's an interesting idea but won't that make things complex to process because you'd need to do meta-annotation processing?

metacosm · 2022-10-07T09:01:59Z

Superseded by #1532

metacosm self-assigned this Sep 2, 2022

metacosm requested a review from csviri September 2, 2022 17:28

csviri force-pushed the next branch from 4e15248 to edd12e5 Compare September 5, 2022 07:45

metacosm added 8 commits September 5, 2022 09:52

refactor: cleaner separation between instantiation and configuration

4fc4452

refactor: move instantiation functionality to Utils

555a738

refactor: unify context information creation, move it to Utils

33a7d3d

refactor: improve getFirstTypeArgumentFromInterface

75822b7

fix: implement configuration method

80fb167

refactor: use new configuration option on SchemaDependentResource

81749c7

fix: deactivate Sonar password inclusion

46dea02

metacosm force-pushed the unify-configuration branch from 6b1a0d4 to 46dea02 Compare September 5, 2022 07:56

Base automatically changed from next to main September 5, 2022 08:14

csviri reviewed Sep 6, 2022

View reviewed changes

metacosm closed this Oct 7, 2022

metacosm deleted the unify-configuration branch October 7, 2022 09:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

WIP: unify configuration #1440

WIP: unify configuration #1440

Uh oh!

metacosm commented Sep 2, 2022

Uh oh!

sonarqubecloud bot commented Sep 5, 2022

Uh oh!

csviri left a comment •

edited

Loading

Uh oh!

csviri Sep 6, 2022

Uh oh!

metacosm Sep 6, 2022

Uh oh!

csviri Sep 6, 2022

Uh oh!

metacosm Sep 6, 2022

Uh oh!

csviri Sep 6, 2022

Uh oh!

metacosm Sep 6, 2022 •

edited

Loading

Uh oh!

csviri Sep 6, 2022

Uh oh!

metacosm Sep 6, 2022

Uh oh!

metacosm commented Sep 6, 2022

Uh oh!

metacosm commented Oct 7, 2022

Uh oh!

Uh oh!

WIP: unify configuration #1440

WIP: unify configuration #1440

Uh oh!

Conversation

metacosm commented Sep 2, 2022

Uh oh!

sonarqubecloud bot commented Sep 5, 2022

Uh oh!

csviri left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csviri Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

metacosm Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

csviri Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

metacosm Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

csviri Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

metacosm Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csviri Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

metacosm Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

metacosm commented Sep 6, 2022

Uh oh!

metacosm commented Oct 7, 2022

Uh oh!

Uh oh!

csviri left a comment •

edited

Loading

metacosm Sep 6, 2022 •

edited

Loading