Refactor DockerInstanceRuntimeInfo#getServers() (#2030) #3282

amisevsk · 2016-12-05T22:42:32Z

What does this PR do?

Enables che-server to communicate directly with wsagent containers when running in Docker through use of a property in che.properties (che.docker.ip.use_internal_address)

Following feedback on #2837, adds interfaces ServerEvaluationStrategyProvider, ServerEvaluationStrategy, and class DefaultServerEvaluationStrategy to replace DockerInstanceRuntimeInfo#getServers(). ServerEvaluationStrategyProvider provides a ServerEvaluationStrategy, which supports the getServer(String portProtocol) method. This simplifies the getServers() method in DockerInstanceRuntimeInfo by abstracting the functionality into a simpler class, while also allowing for extensibility in the future through different implementations of ServerEvaluationStrategy.

What issues does this PR fix or reference?

Fixes #2030, and addresses suggested refactor in #2837.

Previous behavior

Che-server would always ping wsagent on external port, which could cause issues. E.g., if wsagent is available on ports

172.17.0.5:4401 (ephemeral port - within docker0 network)
172.17.0.1:32701 (exposed port - accessible from host network)
then che-server will try to connect to wsagent at 172.17.0.1:32701 in all cases. This connection can be blocked by the firewall in certain cases, preventing workspaces from starting correctly.

New behavior

Che-server can now optionally communicate with wsagent directly (i.e. via 172.17.0.5:4401, above), assuming both containers are on the same docker network. This avoids the issue of the host firewall blocking traffic.

PR Checklist

Tests Added
Tests Passed

amisevsk · 2016-12-05T22:43:38Z

@garagatyi @l0rd Thoughts?

codenvy-ci · 2016-12-05T22:44:02Z

Can one of the admins verify this patch?

garagatyi

This PR is much closer to ideal solution then the previous one. Good job!
Now I figured out what was my mistake when I suggested new design for that. Please elaborate on what you think about suggestion I made in comments.

garagatyi · 2016-12-08T09:31:06Z

assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/codenvy/che.properties

+# Docker network) instead of the docker host address and the port provided by the Docker daemon. May be necessary if the
+# Che server is unable to contact workspace agents, but will prevent communication if the server and workspace
+# are not on the same network.
+che.docker.ip.use_internal_address=false


I think it is better to set property with name of strategy that should be used. Then we can use Guice multibinder and some provider that gets that mulibinder and this property and returns strategy from multibinder configured in this property. I by multibinder I mean Multibinder or even MapBinder

I've changed it so that property refers to strategy name, and strategies are injected in MapBinder.

garagatyi · 2016-12-08T09:42:35Z

...ine/src/main/java/org/eclipse/che/plugin/docker/machine/DefaultServerEvaluationStrategy.java

+        String dockerAddress    = containerInfo.getNetworkSettings().getGateway();
+        this.useEphemeralPorts = true;
+
+        if (internalAddress != null) {


This class suffers from the same problem as in previous PR - it's to complicated and contains different patterns of servers evaluation. I think we can have 2 strategies instead of 1. It can be configurable from a property.
One of the strategies would use ephemeral ports and internal/external hostnames. It can be default one.
Another one would use internal port and container address from containerInfo.getNetworkSettings().getIpAddress(). As far as I understand it is strategy you need for Openshift.

@amisevsk @l0rd WDYT?

I'm split on this -- on one hand, splitting the functionality in DefaultServerEvaluationStrategy into two strategies (default and e.g. local) would simplify the logic in determining which port and address to use, while on the other I feel like the two strategies would be very similar. The majority of the getServers() method would be identical -- we would pretty much just remove the conditional that sets internalAddressAndPort.

I agree with you that using the true/false property is ugly, though. I'd like to hear Mario's thoughts.

For OpenShift, IIRC we actually don't use the new strategy -- we depend on the che.docker.ip.external property.

If we refactor code in such a way that common code would go to some abstract class or something like that strategies would be super simple. Probably in that case default strategy can have logic of ephemeral/exposed port choice.

I like this appoach best of all so far; I've moved most of the functionality into the (now abstract) ServerEvaluationStrategy class. Implementations of ServerEvaluationStrategy now only need to implement getInternalAddressesAndPorts() and getExternalAddressesAndPorts() methods.

garagatyi · 2016-12-08T09:46:11Z

...ine/src/main/java/org/eclipse/che/plugin/docker/machine/DefaultServerEvaluationStrategy.java

+        ServerConfImpl serverConf = getServerConfImpl(portProtocol);
+        if (serverConf.getRef() == null) {
+            // Add default ref to server if it was not set above
+            serverConf.setRef("Server-" + portProtocol.replace('/', '-'));


I think that this code should node be moved out of DockerInstanceRuntimeInfo since it will have to be repeated in different strategies. On the other hand strategy just resolves port and address of the server. References should be added out of a strategy.

The issue I see with that is that then we are setting half of the fields of the ServerImpl in one class (the provider) and the other half in DockerInstanceRuntimeInfo. This is one of the reasons I think the previous implementation was confusing -- I think it's better to get complete ServerImpls when getServers(...) is called.

Even though this means we have to pass the Map<String, ServerConfImpl> to the ServerEvaluationStrategy, I think it's cleaner, because we can directly set internal/externalUrl at the same time as interal/externalAddress. Otherwise, I worry that we're writing this framework to effectively provide two Strings.

What do you think about instead making ServerEvaluationStrategy abstract, and moving getServerConfImpl() there? Then we don't have duplication while still keeping a mostly clean separation between ServerEvaluationStrategy and DockerInstanceRuntimeInfo.

I agree that returning not fully constructed servers is not good design.
Just to clarify I mean that code that evaluates reference, protocol, url, path, internalUrl is the same for different strategies of port/address evaluation. So it would be better to share it between strategies somehow. It can be some method in abstract class or be code in DockerInstanceRuntimeInfo.
I agree that in that case strategies are providers of pair of strings: internal address port, external address port.
Do you have an idea on how we could share that code between strategies and make code clean?

From what I see we need to implement functionality of next 2 methods in some shared code:
https://github.com/garagatyi/che/blob/42272e4bd95c5b9b3c4fc20df5ef60685cac189f/plugins/plugin-docker/che-plugin-docker-machine/src/main/java/org/eclipse/che/plugin/docker/machine/DockerInstanceRuntimeInfo.java#L267-L267
https://github.com/garagatyi/che/blob/42272e4bd95c5b9b3c4fc20df5ef60685cac189f/plugins/plugin-docker/che-plugin-docker-machine/src/main/java/org/eclipse/che/plugin/docker/machine/DockerInstanceRuntimeInfo.java#L255-L255

I've tried to fix this, see the latest commit + message.

garagatyi · 2016-12-08T10:05:39Z

...r-machine/src/main/java/org/eclipse/che/plugin/docker/machine/DockerInstanceRuntimeInfo.java

@@ -234,124 +229,20 @@ public String projectsRoot() {

    @Override
    public Map<String, ServerImpl> getServers() {
+        ServerEvaluationStrategy strategy = provider.getStrategy(info, serversConf, internalHost);


I think that I imaged this code in a wrong way. Because it looks just like I described and it looks not very good.
What do you think if:

Strategies are injected in MapBinder

Some strategy provider is bound as provider of ServersEvaluationStrategy

In constructor ServersEvaluationStrategy is injected

In constructor strategy is assigned to a field.

In getServers we call strategy.getServers(info, internalHost) and it returns Map<String, ServerImpl> with filled ports and addresses.

@l0rd @amisevsk WDYT?

I've tried to implement this in the latest commit -- see message below.

amisevsk · 2016-12-09T22:04:50Z

I've added a commit that changes some things around.

Instead of getServer(portProtocol), the method is now getServers(containerInfo, internalHost, serverConfMap)
ServerEvaluationStrategy is now abstract, and hosts most of the getServers() code.
Implementations of ServerEvaluationStrategy only need to implement abstract getInternalAddressesAndPorts() and getExternalAddressesAndPorts() methods.
Strategy implementations are injected in MapBinder, which is injected into ServerEvaluationStrategyProvider. ServerEvaluationStrategyProvider now implements Provider<ServerEvaluationStrategy>

The tests are currently broken, I can fix them later -- if we're going in this direction then I think DockerInstanceRuntimeInfo#getServers() tests should instead test Strategies directly anyways.

@garagatyi WDYT?

TylerJewell · 2016-12-09T22:05:12Z

Looks great, @amisevsk. I have one addition for you to make. We are about to make the switch so that Docker is the preferred way to run Che. This means that a che.env file will the default way that people configure Che. The template for this has properties that are converted into che.properties overides. An underscore '_' represents a dot '.'. And a double underscore represents an underscore. Can you add in similar docs that you have for che.properties into che.env? I think it belongs either in the networking or in the docker section.

https://github.com/eclipse/che/blob/master/dockerfiles/init/manifests/che.env#L112

amisevsk · 2016-12-09T22:19:16Z

@TylerJewell Thanks for the update! This looks like a great change.

I'll add an entry to che.env -- is there a preferred way to surface the server evaluation strategy options? Should javadoc in classes refer to the property (che.properties) or the environment variable (che.env)?

Also, for my understanding: the new che.env file is generally defining the environment variables that are currently set by the che-launcher docker image, correct?

TylerJewell · 2016-12-12T19:29:16Z

@amisevsk - sorry this slipped through my fingers for a couple days.

To answer your questions:

The che.env is a file that end users will configure that will set all properties, whether they are things that need to go into che.properties or not, as there are environment variables like CHE_HOST which need to be set, but not necessarily into the che.properties. Doing a customer che.properties is hard for end users as well, becuase then they have to figure out how to get it inside the JVM or the container we designated. So we have implemented a system that will take these values and use puppet to transform them into properties if necessary. We also have support for any custom property applied in the che.env file being inserted for the user into a custom che.properties.
So you just need to modify the templated che.env with your custom property, and then test it with the new CLI. To test the new CLI you may need to build it locally.

cd /dockerfiles/base <---- build this
cd /dockerfiles/cli <---- build this

You can then run it:
docker run -it --rm -v /var/run/docker.sock:/var/run/docker.sock -v <path>:/data eclipse/che-cli:nightly start

TylerJewell · 2016-12-15T15:10:13Z

@garagatyi - we need you to review the changes and add any further comment. We are ready to merge, otherwise.

garagatyi

Please address my comments

garagatyi · 2016-12-15T15:23:30Z

...docker-machine/src/main/java/org/eclipse/che/plugin/docker/machine/DockerMachineFactory.java

@@ -81,12 +80,9 @@ DockerNode createNode(@Assisted("workspace") String workspaceId,
     * Creates {@link DockerInstanceRuntimeInfo} instance using assisted injection
     *
     * @param containerInfo description of docker container
-     * @param containerExternalHostname docker host external hostname (used by the browser)
-     * @param containerInternalHostname docker host internal hostname (used by the wsmaster)


please add docs for this parameter

garagatyi · 2016-12-15T15:31:12Z

...ne/src/main/java/org/eclipse/che/plugin/docker/machine/ServerEvaluationStrategyProvider.java

+        if (strategies.containsKey(chosenStrategy)) {
+            this.strategy = strategies.get(chosenStrategy);
+        } else {
+            LOG.warn(String.format("Property che.docker.server_evaluation_strategy=%s"


It will be hard to debug and realize what happened if developer made a typo in strategy name for example. So it is better to fail component start in that case instead of usage of default strategy.

That makes a lot of sense.

What is the preferred way of doing this in Che? I've changed it to throw a ServerException with a message now, but the error that appears while launching a workspace includes errors about being unable to inject the constructor, which is not very clean. Since I'm implementing Provider<ServerEvaluationStrategy>, we can't throw an exception in the get() method. I suppose implementing Provider is not strictly required, WDYT?

No, it can be provider de-facto but doesn't implement any interface

The further issue that MachineRuntimeInfo (and thus DockerInstanceRuntimeInfo) do not leave space for throwing a ServerException. Is there an internal way to signal an error otherwise?

Alternatively, we could add throws declarations to MachineRuntimeInfo, and adjust as necessary, but DockerInstanceRuntimeInfo is used pretty widely in the codebase.

I think that declaring throwing of an exception in MachineRuntimeInfo is better to omit because, as you said, it is used in a lot of places and bothering all that code with catching exception would make a lot of code much more complicated.

So I think we need to find another way to achieve what you want.

Can you elaborate on what problem you have faced with lack of exception throwing?

For useability, it would be ideal if typing the property incorrectly would cause workspaces to fail to start, displaying a message saying something along the lines of "Server Evaluation Strategy <property> does not correspond to any implemented strategy". The user could read their typo, and make the appropriate fix in che.env without too much trouble.

I'm not familiar enough with the relevant sections of the code to see an easy way to fail workspace starting and also display this message to the user without throwing an exception. This works relatively well, except that the only place I can throw this exception is in the ServerEvaluationStrategyProvider constructor, which litters the error message with "failed to inject constructor" and other information about the internals of Che.

My initial approach was to add a error message to the log and use the default strategy, but I agree that this can easily lead to confusion. Another option is to log an error and use a no-op implementation of ServerEvaluationStrategy -- this would cause launching a workspace to fail, but the user would still have to view the logs to find their error.

How would you like me to surface this error to the user?

If you set this class to be eager singleton Guice will have to create it on Che bootstrap and mistake in strategy name would prevent successful start of Che. Maybe this variant is ok for you?
In addition to that check and error throwing can be moved to @PostConstruct method. Then maybe message of Che start fail would be more satisfactory.

After playing around with it for awhile, I think I've got something that works.

The Exception is thrown in a @PostConstruct method, which causes workspace startup to fail and display a user-readable message. Binding ServerEvaluationStrategyProvider as a Singleton caused an issue where Che would still start without displaying errors (except for in the logs), but be in an inconsistent state and have to be stopped from the docker cli directly.

garagatyi · 2016-12-15T15:33:45Z

...er-machine/src/main/java/org/eclipse/che/plugin/docker/machine/ServerEvaluationStrategy.java

+                                                                        String internalAddress);
+
+    @Inject
+    protected ServerEvaluationStrategy(@Nullable @Named("che.docker.ip")          String internalAddressProperty,


We put constructor before other methods in Che source code. Please follow that to not distract other devs.

garagatyi · 2016-12-15T15:36:54Z

...er-machine/src/main/java/org/eclipse/che/plugin/docker/machine/ServerEvaluationStrategy.java

+
+        Map<String, ServerImpl> servers = new LinkedHashMap<>();
+
+        for (String portProtocol : portBindings.keySet()) {


Please add unit tests for that class

Most of the getServers() logic is tested in DockerInstanceRuntimeInfoTest, as part of the tests there. I can move it out if that's preferred, but I think of ServerEvaluationStrategy as tightly coupled with DockerInstanceRuntimeInfo.

If something is implemented in base class it is better to test that in tests for base class. Then we won't have to test that in all implementations

garagatyi · 2016-12-15T15:37:51Z

...er-machine/src/main/java/org/eclipse/che/plugin/docker/machine/ServerEvaluationStrategy.java

+            ServerConfImpl serverConf = getServerConfImpl(portProtocol, labels, serverConfMap);
+
+            // Add protocol and path to internal/external address, if applicable
+            String internalUrl = null, externalUrl = null;


Please do not declare several variables in one line. We don't use such syntax in Che.

garagatyi · 2016-12-15T15:42:18Z

...src/main/java/org/eclipse/che/plugin/docker/machine/LocalDockerServerEvaluationStrategy.java

+
+        String internalAddress;
+        boolean useExposedPorts = true;
+        if (internalAddressContainer != null && !internalAddressContainer.isEmpty()) {


Use IsNullOrEmpty - with it code usually looks clearer

Fixed throughout. Thank you.

garagatyi · 2016-12-15T15:44:14Z

...src/main/java/org/eclipse/che/plugin/docker/machine/LocalDockerServerEvaluationStrategy.java

+        if (internalAddressContainer != null && !internalAddressContainer.isEmpty()) {
+            internalAddress = internalAddressContainer;
+        } else {
+            internalAddress = internalHost;


If internal host provided from another component why it is not prevail?

Internal Host is a holdover from before -- it ultimately comes from DockerNode#getHost(). Prior to these changes, this value was used only when running Che Server as a standalone tomcat server. I've left it here as a fallback, but it could probably be removed from LocalDockerServerEvaluationStrategy altogether. It has lowest precedence because that was the way it was used before: it's the same as internalHostnameAssisted here. Note that when running Che using che-launcher, the env variable CHE_DOCKER_MACHINE_HOST_INTERNAL is set to the value defined here

The code that defines internalHost is in DockerConnectorConfiguration.

garagatyi · 2016-12-15T15:44:42Z

...src/main/java/org/eclipse/che/plugin/docker/machine/LocalDockerServerEvaluationStrategy.java

+            internalAddress = internalAddressContainer;
+        } else {
+            internalAddress = internalHost;
+            useExposedPorts = false;


Can you elaborate on why in that case strategy uses ephemeral ports?

We need to use ephemeral ports whenever we are not using the direct IP address of the workspace container. Internal host requires ephemeral ports for communication, but if you think it can safely be removed from this strategy, this could be simplified.

garagatyi · 2016-12-15T15:46:37Z

...src/main/java/org/eclipse/che/plugin/docker/machine/LocalDockerServerEvaluationStrategy.java

+    }
+
+    @Override
+    protected Map<String, String> getExternalAddressesAndPorts(ContainerInfo containerInfo, String internalHost) {


Please add unit tests for this class

I've rewritten ServerEvaluationStrategyTest.java to be more clear, it tests getInternalAddress() and getExternalAddress() for the strategies.

garagatyi · 2016-12-15T15:48:04Z

...ine/src/main/java/org/eclipse/che/plugin/docker/machine/DefaultServerEvaluationStrategy.java

+    }
+
+    @Override
+    protected Map<String, String> getExternalAddressesAndPorts(ContainerInfo containerInfo, String internalHost) {


Please add unit tests for that class

I've rewritten ServerEvaluationStrategyTest.java to be more clear, it tests getInternalAddress() and getExternalAddress() for the strategies.

TylerJewell

I thought about it, Angel, and could not identify a better name. So let's roll with it!

bmicklea · 2016-12-20T15:48:37Z

@amisevsk we're in ramp down for M9 (our GA release), we've moved the milestone to 5.1 for now but if you're able to get the conflicts resolved and docs and tests added in time we'll add it to M9.

garagatyi

Good job! This is really big and complex contribution! And I believe it really simplifies code.
The only thing needed is separation of strategies tests into separate test classes. I provided more details in code comment.

Please also squash your git history to avoid pushing commits that are parts of a single task and can not be used separately.

garagatyi · 2016-12-21T10:55:21Z

...achine/src/test/java/org/eclipse/che/plugin/docker/machine/ServerEvaluationStrategyTest.java

+     * @throws Exception
+     */
+    @Test
+    public void defaultStrategyShouldUseInternalIpPropertyToOverrideContainerInfo() throws Exception {


Please test here only code and behavior belonging to ServerEvaluationStrategy. Since it is abstract you can use test implementation that extends this class. Each real implementation should have its own test class that tests only behavior of that implementation.
It may be useful to mock or spy some class or fake it in another way to test in specific tests only needed class. So what is tested in base class doesn't have to be tested in child class

Fixed, thank you.

garagatyi · 2016-12-21T11:11:52Z

@musienko-maxim Please run QA cycle for that PR.

musienko-maxim · 2016-12-21T13:51:26Z

Can you resolve all conflicts? QA team will try to launch qa session after this

amisevsk · 2016-12-21T19:43:48Z

I've fixed the tests and cleared the merge conflict.

There is one outstanding issue, of signalling that che.docker.server_evaluation_strategy has a typo, that is outstanding. I would like @garagatyi 's input on how to best deal with this case. Once that is dealt with I will squash these commits into a few logical chunks.

Apart from that, the ip-validation check is failing due to commit 9186d0c, but that will be fixed when the history is squashed.

Finally, current builds fail without the -Dskip-validate-sources flag, due to issue #3281. I'm not sure about how best to proceed there.

@musienko-maxim Is anything else needed before QA can begin?

musienko-maxim · 2016-12-22T07:58:21Z

Thanks! It's enough.

garagatyi · 2016-12-22T09:53:43Z

Unfortunately we can't merge PR with build failing because of source validation. So options here are:

waiting until Build is forcing all contributions to be copyrighted to Codenvy #3281 is fixed
disable license validation for some files in pom.xml, merge PR, wait fix of Build is forcing all contributions to be copyrighted to Codenvy #3281, remove disabled license validation

WDYT?

amisevsk · 2016-12-23T23:13:04Z

I've squashed the history into three commits:
- The first adds ServerEvaluationStrategy, the Provider, and implmentations,
- The second adds test cases for ServerEvaluationStrategy
- The third changes existing files to use ServerEvaluationStrategy.

I can squash these further, but I feel like having three separate steps means that the history is more readable in general.

@garagatyi I'm happy to add excludes entries for the problem files and make a note to remove them after #3281 is resolved. I assume that the preferred location for this in the che-parent pom.xml, correct?

garagatyi · 2016-12-24T08:05:59Z

You can leave these 3 commits.

You need to edit nearest pom.xml to exclude file from license check. Take a look at this pom

…heck The current license checking maven plugin does not allow for multiple copyright owners on source files. This commit adds files modified for ServerEvaluationStrategy to an excludes list so that builds can continue normally. This commit should be undone once issue eclipse-che#3281 is resolved. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

amisevsk · 2016-12-30T23:06:51Z

@garagatyi Sorry about the delay, I've fixed the javadocs and moved the internalAddressProperty and externalAddressProperty into the concrete classes. The changes have been squashed into commit e5bc686.

garagatyi · 2017-01-02T18:28:25Z

I'm also on holidays )
Thanks that you addressed my comments. These were optional, just advises on how to make code clearer.

…heck The current license checking maven plugin does not allow for multiple copyright owners on source files. This commit adds files modified for ServerEvaluationStrategy to an excludes list so that builds can continue normally. This commit should be undone once issue eclipse-che#3281 is resolved. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

amisevsk · 2017-01-05T17:52:12Z

I've rebased to fix merge conflicts and updated header copyright years. It looks like IP validation is failing due to lack of Signed-off-by footers in master.

@garagatyi Is there anything else I can do to help with merging this change?

l0rd · 2017-01-05T18:30:32Z

@amisevsk I think you merged commit f8ba55f in your branch instead of rebasing on top of it

Adds abstract class ServerEvaluationStrategy which can be used to change how Che Server communicates with workspace containers. ServerEvaluationStrategy is meant to be extended to modify the behavior of DockerInstanceRuntimeInfo#getServers(). Two implementations of ServerEvaluationStrategy are included: DefaultServerEvaluationStrategy (which is identical to normal getServers() functionality) and LocalDockerServerEvaluationStrategy, which uses internal container addresses for workspace containers and can help in cases where firewall is an issue. Strategies are provided by ServerEvaluationStrategyProvider, which uses the new property che.docker.server_evaluation_strategy to choose which implementation is provided. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Add tests for ServerEvaluationStrategy, DefaultServerEvaluationStrategy, and LocalDockerServerEvaluationStrategy Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Replaces DockerInstanceRuntimeInfo#getServers() to use ServerEvaluationStrategy. Deletes LocalDockerInstanceRuntimeInfo class as it is no longer needed. Adds MapBinder of ServerEvaluationStrategy to LocalDockerModule. Updates DockerInstanceRuntimeInfo tests to be more readable and removes now unnecessary tests. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

…heck The current license checking maven plugin does not allow for multiple copyright owners on source files. This commit adds files modified for ServerEvaluationStrategy to an excludes list so that builds can continue normally. This commit should be undone once issue eclipse-che#3281 is resolved. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

amisevsk · 2017-01-05T18:55:15Z

@l0rd Woops! Thank you, I don't know how that happened. It should be fixed now.

garagatyi · 2017-01-06T08:08:42Z

@amisevsk Since this PR is tagged with 5.1 milestone we can't merge it now. We will have to wait for 5.0 release. Then I'll ask QA team to re-run QA cycle (previous QA on this PR passed) to ensure that changes in master made since previous QA cycle didn't brake anything in your PR.

TylerJewell · 2017-01-09T13:50:40Z

@amisevsk - 5.0.0 is released. @garagatyi, @l0rd - lets go ahead and merge this now early in our cycle, and we'll have QA execute tests within the master branch. We have a window of a few days to merge major PRs for 5.1 and do not want to wait for the QA cycle.

amisevsk · 2017-01-10T13:28:35Z

@garagatyi @TylerJewell Thanks! Let me know if there are any QA issues during the next cycle.

garagatyi · 2017-01-10T13:39:29Z

@amisevsk Thank you that you were patient with all my comments in review. But now code looks muuuch better than before your contribution!

…heck The current license checking maven plugin does not allow for multiple copyright owners on source files. This commit adds files modified for ServerEvaluationStrategy to an excludes list so that builds can continue normally. This commit should be undone once issue eclipse-che#3281 is resolved. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Refactor DockerInstanceRuntimeInfo#getServers() (eclipse-che#2030)

amisevsk mentioned this pull request Dec 5, 2016

Che server should connect to ws-agent on internal URL (#2030) #2837

Closed

6 tasks

garagatyi suggested changes Dec 8, 2016

View reviewed changes

bmicklea added this to the 5.0.0-M9 milestone Dec 12, 2016

TylerJewell approved these changes Dec 15, 2016

View reviewed changes

garagatyi suggested changes Dec 15, 2016

View reviewed changes

TylerJewell approved these changes Dec 15, 2016

View reviewed changes

benoitf force-pushed the master branch from f0ee59a to aba9a87 Compare December 20, 2016 09:31

bmicklea added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Dec 20, 2016

JamesDrummond modified the milestones: 5.1.0, 5.0.0-M9 Dec 20, 2016

garagatyi suggested changes Dec 21, 2016

View reviewed changes

garagatyi approved these changes Dec 22, 2016

View reviewed changes

amisevsk force-pushed the CHE-2030-refactor branch from 4e8b81a to cae95cd Compare December 23, 2016 23:03

amisevsk force-pushed the CHE-2030-refactor branch from cae95cd to 0e0e1f5 Compare December 24, 2016 06:35

amisevsk force-pushed the CHE-2030-refactor branch from 08ba22f to 9d1f7c0 Compare December 30, 2016 23:01

amisevsk force-pushed the CHE-2030-refactor branch from 9d1f7c0 to b9fa63b Compare January 5, 2017 17:04

amisevsk force-pushed the CHE-2030-refactor branch from b9fa63b to 5c3f0ef Compare January 5, 2017 17:49

amisevsk added 4 commits January 5, 2017 13:52

CHE-2030: Add tests for ServerEvaluationStrategy

f7d247a

Add tests for ServerEvaluationStrategy, DefaultServerEvaluationStrategy, and LocalDockerServerEvaluationStrategy Signed-off-by: Angel Misevski <amisevsk@redhat.com>

amisevsk force-pushed the CHE-2030-refactor branch from 5c3f0ef to 3473c01 Compare January 5, 2017 18:54

l0rd approved these changes Jan 5, 2017

View reviewed changes

l0rd mentioned this pull request Jan 9, 2017

Che on a cloud server - Not able to create a workspace #3576

Closed

garagatyi merged commit 33a4d07 into eclipse-che:master Jan 10, 2017

amisevsk mentioned this pull request Jan 12, 2017

Update Setup docs to reflect changes from Che PR 3282 eclipse-che/che-docs#61

Merged

benoitf mentioned this pull request Jan 16, 2017

Fix #3686 : Property around external docker has been changed in a refactoring #3753

Merged

amisevsk mentioned this pull request Jan 18, 2017

Che nightly does not start on Windows 10 #3686

Closed

JamesDrummond mentioned this pull request Jan 31, 2017

5.1.0 Milestone Overview #3972

Closed

9 tasks

JPinkney pushed a commit to JPinkney/che that referenced this pull request Aug 17, 2017

Merge pull request eclipse-che#3282 from amisevsk/CHE-2030-refactor

25d87ef

Refactor DockerInstanceRuntimeInfo#getServers() (eclipse-che#2030)

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 Nov 2, 2017


		Map<String, ServerImpl> servers = new LinkedHashMap<>();

		for (String portProtocol : portBindings.keySet()) {

Refactor DockerInstanceRuntimeInfo#getServers() (#2030) #3282

Refactor DockerInstanceRuntimeInfo#getServers() (#2030) #3282

Conversation

amisevsk commented Dec 5, 2016

What does this PR do?

What issues does this PR fix or reference?

Previous behavior

New behavior

PR Checklist

amisevsk commented Dec 5, 2016

codenvy-ci commented Dec 5, 2016

garagatyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amisevsk commented Dec 9, 2016

TylerJewell commented Dec 9, 2016

amisevsk commented Dec 9, 2016

TylerJewell commented Dec 12, 2016

TylerJewell commented Dec 15, 2016

garagatyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerJewell left a comment

Choose a reason for hiding this comment

bmicklea commented Dec 20, 2016

garagatyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garagatyi commented Dec 21, 2016

musienko-maxim commented Dec 21, 2016

amisevsk commented Dec 21, 2016

musienko-maxim commented Dec 22, 2016

garagatyi commented Dec 22, 2016

amisevsk commented Dec 23, 2016

garagatyi commented Dec 24, 2016

amisevsk commented Dec 30, 2016

garagatyi commented Jan 2, 2017

amisevsk commented Jan 5, 2017

l0rd commented Jan 5, 2017

amisevsk commented Jan 5, 2017

garagatyi commented Jan 6, 2017