Skip to content

simplify operator instantiation #263

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 9 commits into from
Closed

Conversation

s-soroosh
Copy link
Contributor

No description provided.

@s-soroosh s-soroosh changed the title [WIP] simplify operator instantiation simplify operator instantiation Dec 18, 2020
@metacosm
Copy link
Collaborator

I'm not sure about this one, to be honest… It seems to complicate things for not much benefit. Is needing to call to the default configuration service really that big of an issue? Why wouldn't we also hide the DefaultKubernetesClient as well?

@s-soroosh
Copy link
Contributor Author

s-soroosh commented Dec 18, 2020

I'm not sure about this one, to be honest… It seems to complicate things for not much benefit. Is needing to call to the default configuration service really that big of an issue? Why wouldn't we also hide the DefaultKubernetesClient as well?

what did you find complicated here? what I did was just a Dependency inversion to ensure the clients are not bound to actual implementation.

Why wouldn't we also hide the DefaultKubernetesClient as well?

The StandaloneOperator is dependent on KubernetesClient, the user can use different implementations like OpenshiftClient. But ConfigurationService is a thing introduced by us to support different environments, I don't think users need to be aware of it unless they have advanced feature. i.e. they can use AbstractOperator.

@s-soroosh s-soroosh requested a review from kirek007 December 22, 2020 22:34
@adam-sandor
Copy link
Collaborator

Can you provide a description of what this change intends to accomplish? By reading the code I don't see the point.

@s-soroosh
Copy link
Contributor Author

Can you provide a description of what this change intends to accomplish? By reading the code I don't see the point.

Sure, by this changeset the Operator becomes an interface which has different implementations for different environments.

From the user pov in pure-java they can instantiate the operator like this:

       Operator operator = new StandaloneOperator(new DefaultKubernetesClient());
       operator.register(new WebServerController());

i.e they don't need to know about the configuration services the SDK has.
In future, we can have implementations like MockOperator or more specific implementation for frameworks where they support native monitoring, health check and so forth.

@adam-sandor
Copy link
Collaborator

I'm still missing some good examples of where this change will bring benefits. The current changes in examples are just a change in what class gets instantiated.

@s-soroosh
Copy link
Contributor Author

s-soroosh commented Jan 2, 2021

I'm still missing some good examples of where this change will bring benefits. The current changes in examples are just a change in what class gets instantiated.

@adam-sandor It's not just that, the required ctor parameters is also less.

Operator operator = new Operator(new DefaultKubernetesClient(), DefaultConfigurationService.instance());

vs

Operator operator = new StandaloneOperator(new DefaultKubernetesClient());

@metacosm
Copy link
Collaborator

Closing this as obsolete.

@metacosm metacosm closed this Aug 31, 2021
@metacosm metacosm deleted the abstract-operator branch August 31, 2021 19: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.

3 participants