Skip to content
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

Merged
merged 2 commits into from
Sep 11, 2018

Conversation

garagatyi
Copy link

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

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>
@benoitf benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/task Internal things, technical debt, and to-do tasks to be performed. labels Sep 7, 2018
@garagatyi
Copy link
Author

ci-test

@garagatyi garagatyi changed the title [WIP] Rework plugin brokering to reuse PVC strategies code Rework plugin brokering to reuse PVC strategies code Sep 10, 2018
* @author Oleksandr Garagatyi
*/
@Beta
public class KBrokerEnvironmentConfig extends BrokerEnvironmentConfig<KubernetesEnvironment> {
Copy link
Contributor

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?

Copy link
Author

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> {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenShiftBrokerEnvironmentConfig?

Copy link
Author

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

Copy link
Member

@sleshchenko sleshchenko left a 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> {
Copy link
Member

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.

Copy link
Author

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. */
Copy link
Member

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@riuvshin
Copy link
Contributor

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:11119
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@garagatyi
Copy link
Author

@eclipse/eclipse-che-qa please, take a look

@artaleks9
Copy link
Contributor

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.

@garagatyi garagatyi merged commit 24ccece into eclipse-che:master Sep 11, 2018
@garagatyi garagatyi deleted the skeletonPVC2 branch September 11, 2018 12:48
@benoitf benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Sep 11, 2018
@benoitf benoitf added this to the 6.11.0 milestone Sep 11, 2018
@vkuznyetsov vkuznyetsov mentioned this pull request Sep 11, 2018
32 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed.
Projects
None yet
6 participants