Skip to content

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

Closed
wants to merge 6 commits into from

Conversation

metacosm
Copy link
Collaborator

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.

@@ -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);
Copy link
Contributor

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 in InitMethod?

Copy link
Collaborator Author

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.

Copy link
Collaborator

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).

Copy link
Collaborator Author

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… 😀

Copy link
Collaborator

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

Copy link
Contributor

@s-soroosh s-soroosh Nov 18, 2020

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.

@metacosm metacosm changed the title feat: make it possible to externally configure the operator WIP: feat: make it possible to externally configure the operator Nov 18, 2020
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.

4 participants