-
Notifications
You must be signed in to change notification settings - Fork 220
WIP: feat: make it possible to externally configure the operator #228
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
@@ -27,5 +28,6 @@ | |||
* <b>However we will always call an update if there is no finalizer on object and its not marked for deletion.</b> | |||
*/ | |||
UpdateControl<R> createOrUpdateResource(R resource, Context<R> context); | |||
|
|||
|
|||
void setClient(KubernetesClient client); |
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.
I would challenge adding a setter method signature into the interfaces in general.
and specifically here IMO
- The usage is not really simpler than getting the client via constructor if it's really needed
- The KubernetesClient is an implementation dependency and not really specification, I can imagine cases that developers would like to use any other client or even a controller that doesn't need a KubernetesClient at all.
- Setting the value via setter makes it unpredictable about when the object state is ready to use. e.g. what if some reasons developer wants to access the
client
in the constructor or inInitMethod
?
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.
I would challenge adding a setter method signature into the interfaces in general.
I agree unless that is a part of the expected behavior of the implementations to have that information provided to them.
and specifically here IMO
* The usage is not really simpler than getting the client via constructor **if it's really needed**
Agreed in general. That said, the goal here to be able to configure the client externally to the app once and then retrieve that instance where it's needed. This is still a work in progress. A next step would be to provide automatic injection of the client.
* The KubernetesClient is an implementation dependency and not really specification, I can imagine cases that developers would like to use any other client or even a controller that doesn't need a KubernetesClient at all.
This project is a framework built on top of the KubernetesClient with the aim of easing using said client to create operators. I'd argue that most implementations would need a client injected to do their work. If your controller doesn't need the client, why use this framework at all? Do you have examples of cases where it would make sense to use this framework and not require a client instance (test fixture excepted)?
* Setting the value via setter makes it unpredictable about when the object state is ready to use. e.g. what if some reasons developer wants to access the `client` in the constructor or in `InitMethod`?
What's preventing implementations from retrieving the client from the operator instance and use it as they see fit (or even create a separate one if needed)? I do agree that it makes the ready state of the controller a little less clear in some instances but the aim of this work is to streamline the most common use case where the controller will only be used after it has been registered with the operator.
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.
I agree with @psycho-ir , we don't really get too much adding this to the interface. In an operator we register an instance of a controller. So who implements the operator can prepare this instance as he wants.
In most of the cases we need the client (except when we manage only non-kuberntetes 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.
I agree that using a setter is sub-par. The proper solution would be to be able to use injection so that the client is automatically retrieved where it's needed but I didn't want to go there just yet… 😀
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.
I get where you're coming from @metacosm I keep having the same thought that this should be more controlled. But then I always have to admit that it's just so straightforward to get the client into the controller in the main method that it doesn't make sense to complicate the framework with this. We do have a serious issue with initializing the custom resource client that happens during controller registration but there is no way to get it into the controller in a nice way. You see that here: https://github.com/java-operator-sdk/tomcat-operator/blob/master/src/main/java/com/example/jaxdemo/TomcatOperator.java
and this is the issue for it: #199
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.
I agree that using a setter is sub-par. The proper solution would be to be able to use injection so that the client is automatically retrieved where it's needed but I didn't want to go there just yet… 😀
Given that using a preferred DI by the developer for an operator is a no brainer task, I doubt whether it makes sense to implement functionality as part of the core or not.
The operator gets a client based on the configuration. ResourceControllers now get their client from the operator when they get registered. Spring Boot starter needs to get adapted.
The operator gets a client based on the configuration.
ResourceControllers now get their client from the operator when they get
registered. Spring Boot starter needs to get adapted.