-
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
CHE-26: Initial OpenShift integration #3798
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
/******************************************************************************* | ||
* Copyright (c) 2012-2017 Codenvy, S.A. | ||
* All rights reserved. This program and the accompanying materials | ||
* are made available under the terms of the Eclipse Public License v1.0 | ||
* which accompanies this distribution, and is available at | ||
* http://www.eclipse.org/legal/epl-v10.html | ||
* | ||
* Contributors: | ||
* Codenvy, S.A. - initial API and implementation | ||
*******************************************************************************/ | ||
package org.eclipse.che.plugin.docker.client; | ||
|
||
import java.util.Map; | ||
|
||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
import com.google.inject.Inject; | ||
import com.google.inject.Provider; | ||
import com.google.inject.Singleton; | ||
import com.google.inject.name.Named; | ||
|
||
@Singleton | ||
public class DockerConnectorProvider implements Provider<DockerConnector> { | ||
|
||
private static final Logger LOG = LoggerFactory.getLogger(DockerConnectorProvider.class); | ||
private DockerConnector connector; | ||
|
||
@Inject | ||
public DockerConnectorProvider(Map<String, DockerConnector> connectors, | ||
@Named("che.docker.connector") String property) { | ||
if (connectors.containsKey(property)) { | ||
this.connector = connectors.get(property); | ||
} else { | ||
LOG.warn("Property 'che.docker.connector' did not match any bound implementation of DockerConnector. Using default."); | ||
LOG.warn("\t Value of 'che.docker.connector': {}", property); | ||
LOG.warn("\t Bound implementations: {}", connectors.toString()); | ||
this.connector = connectors.get("default"); | ||
} | ||
} | ||
|
||
@Override | ||
public DockerConnector get() { | ||
return connector; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,7 @@ | |
import org.eclipse.che.commons.annotation.Nullable; | ||
import org.eclipse.che.commons.lang.NameGenerator; | ||
import org.eclipse.che.plugin.docker.client.DockerConnector; | ||
import org.eclipse.che.plugin.docker.client.DockerConnectorProvider; | ||
import org.eclipse.che.plugin.docker.client.Exec; | ||
import org.eclipse.che.plugin.docker.client.LogMessage; | ||
import org.eclipse.che.plugin.docker.client.ProgressLineFormatterImpl; | ||
|
@@ -104,7 +105,7 @@ public class DockerInstance extends AbstractInstance { | |
private final MachineRuntimeInfoImpl machineRuntime; | ||
|
||
@Inject | ||
public DockerInstance(DockerConnector docker, | ||
public DockerInstance(DockerConnectorProvider dockerProvider, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need guice Providers vs named connector ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I got the comment :( We do need to inject provider because now it is in charge of providing suitable connector (DockerConnector / OpenShiftConnector) based on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @benoitf BTW, similar approach is used for obtaining ServerEvaluationStrategy (default / docker-local) - https://github.com/eclipse/che/blob/master/plugins/plugin-docker/che-plugin-docker-machine/src/main/java/org/eclipse/che/plugin/docker/machine/local/LocalDockerModule.java#L52 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ibuziuk I agree There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with Florent that it would look prettier if we get rid of injecting provider. There is always a way to reduce distraction. But up to you. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @garagatyi For initial implementation I would rather leave it this way. The usage of |
||
@Named("che.docker.registry") String registry, | ||
@Named("che.docker.namespace") @Nullable String registryNamespace, | ||
DockerMachineFactory dockerMachineFactory, | ||
|
@@ -119,7 +120,7 @@ public DockerInstance(DockerConnector docker, | |
super(machine); | ||
this.dockerMachineFactory = dockerMachineFactory; | ||
this.container = container; | ||
this.docker = docker; | ||
this.docker = dockerProvider.get(); | ||
this.image = image; | ||
this.outputConsumer = outputConsumer; | ||
this.registry = registry; | ||
|
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.
Why do you prefer setting default implementation here instead of che.properties?
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.
@garagatyi oh, the property was just named wrongly the logs :/ . The default implementation is set in
che.properties
- https://github.com/eclipse/che/pull/3798/files#diff-5bbadd86c05d20f13241983c27be08b2R283However, if it does not match any supported values (openshift / default) default
DockerConnector
implementation is used