-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Rework plugin brokering to reuse PVC strategies code #11119
Conversation
Rework plugin broker code to reuse PVC strategies code. Moreover, it applies other OS/K8s provisioners with proxy, name uniqueness, private docker image registry, pod termination support. Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
3028999
to
fe59fc0
Compare
ci-test |
* @author Oleksandr Garagatyi | ||
*/ | ||
@Beta | ||
public class KBrokerEnvironmentConfig extends BrokerEnvironmentConfig<KubernetesEnvironment> { |
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.
what about names K8sBrokerEnvironmentConfig or KubernetesBrokerEnvironmentConfig?
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'll rename it to KubernetesBrokerEnvironmentFactory
*/ | ||
@Beta | ||
public class OBrokerEnvironmentConfig extends BrokerEnvironmentConfig<OpenShiftEnvironment> { | ||
|
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.
OpenShiftBrokerEnvironmentConfig?
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'll rename it to OpenshiftBrokerEnvironmentFactory
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.
Other LGTM
* @author Oleksandr Garagatyi | ||
*/ | ||
@Beta | ||
public abstract class BrokerEnvironmentConfig<E extends KubernetesEnvironment> { |
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 think BrokerEnvironmentFactory
would be better name here since it holds broker environment config but the only public method that is available is <E extends KubernetesEnvironment> create(....)
. I understand that there is a collision with InternalEnvironmentFactories. So, please consider finding a more suitable name.
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'll rename it to BrokerEnvironmentFactory
return doCreate(machineName, machine, configMapName, configMap, pod); | ||
} | ||
|
||
/** Needed to reuse this component in both - Kubernetes and Openshift infrastructures. */ |
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 say that it should not be reused in both infrastructures but reimplemented.
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.
+1
ci-test build report: |
@eclipse/eclipse-che-qa please, take a look |
Selenium tests execution on Eclipse Che Multiuser on OCP (https://ci.codenvycorp.com/job/che-pullrequests-test-ocp/762/) doesn't show any regression against this Pull Request. |
What does this PR do?
Rework plugin broker code to reuse PVC strategies code.
Moreover, it applies other OS/K8s provisioners with proxy, name uniqueness, private docker image registry, pod termination support.
What issues does this PR fix or reference?
Fixes #10879
Release Notes
Docs PR