Skip to content

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

Merged
merged 2 commits into from
Jan 31, 2022

Conversation

metacosm
Copy link
Collaborator

  • 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.
@metacosm metacosm self-assigned this Jan 28, 2022
@metacosm metacosm requested a review from csviri January 28, 2022 23:16
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell A 21 Code Smells

29.6% 29.6% Coverage
0.8% 0.8% Duplication


Class<? extends DependentResource<R, P>> getDependentResourceClass();

Class<R> getResourceClass();
Copy link
Collaborator

@csviri csviri Jan 31, 2022

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.

Copy link
Collaborator Author

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> {
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

@metacosm metacosm merged commit 45b187d into next Jan 31, 2022
@metacosm metacosm deleted the more-config branch January 31, 2022 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants