Fix broken links section in docker-compose recipe #2850

mmorhun · 2016-10-20T16:23:28Z

What does this PR do?

Fix broken links section in docker-compose recipe.

What issues does this PR fix or reference?

#2687

Previous behavior

If links section is present in recipe than workspace will fail to start.

New behavior

Links and aliases works now.

Signed-off-by: Mykola Morhun <[email protected]>

mmorhun · 2016-10-20T16:24:35Z

@garagatyi @benoitf @l0rd @AndrienkoAleksandr review please

mmorhun · 2016-10-20T16:31:54Z

I'll add tests a bit later, maybe in M7

AndrienkoAleksandr · 2016-10-20T16:32:11Z

+1

codenvy-ci · 2016-10-20T18:22:42Z

Build # 755 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/755/ to view the results.

gazarenkov · 2016-10-21T07:13:21Z

What is the reason of names like "...CorrectSource"?
It is supposed to have some "...IncorrectSources" and things as well?
Things like this makes the code less readable

mmorhun · 2016-10-21T11:21:47Z

Agree, I'll change the name.

Signed-off-by: Mykola Morhun <[email protected]>

benoitf

For me, the tests should be added before merging as I don't see how we can be sure without testing.

benoitf · 2016-10-21T13:06:20Z

...n-docker-client/src/main/java/org/eclipse/che/plugin/docker/client/json/NetworkSettings.java

@@ -17,7 +17,7 @@

 /** @author andrew00x */
 public class NetworkSettings {
-    private String   ipAddress;
+    private String   iPAddress;


I would keep ipAddress as it was as method name is getIpAddress() not getIPAddress()

Yes, ipAddress is conventional name , but iPAddress is needed for parsing value from json. Exactly like iPPrefixLen, iPv6Gateway in the same class.

I agree with @benoitf that iPAddress is weird. Wouldn't it possible to use @SerializedName annotation instead?

benoitf · 2016-10-21T13:09:05Z

...api-workspace/src/main/java/org/eclipse/che/api/environment/server/CheEnvironmentEngine.java

+     */
+    @VisibleForTesting
+    void replaceServiceToContainerNameInLinks(CheServiceImpl serviceToStart, List<CheServiceImpl> launchedServices) {
+        final List<String> containerLinks = serviceToStart.getLinks();


we couldn't use Java8 stream/filter/collect for the whole method ?

Yes, we can, but I think that it is redundant here.
If we used stream here it would made code more complicated and we would had additional expenses for stream creation/collection here (note, that in many cases list with links contains 0 or 1 element, i.e. how often and how many links you use in docker compose?)

codenvy-ci · 2016-10-21T13:56:21Z

Build # 762 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/762/ to view the results.

Signed-off-by: Mykola Morhun <[email protected]>

codenvy-ci · 2016-10-24T18:05:00Z

Build # 777 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/777/ to view the results.

l0rd · 2016-10-25T08:39:48Z

...api-workspace/src/main/java/org/eclipse/che/api/environment/server/CheEnvironmentEngine.java

+    /**
+     * Replaces linked to this service's name with container name which represents the service in links section.
+     * The problem is that a user writes names of other services in links section in compose file.
+     * But actually links are constraints and they values should be names of containers (not services) to be linked.


and they values => and their values

l0rd · 2016-10-25T08:46:47Z

...api-workspace/src/main/java/org/eclipse/che/api/environment/server/CheEnvironmentEngine.java

+    void replaceServiceToContainerNameInLinks(CheServiceImpl serviceToStart, Map<String, CheServiceImpl> services) {
+        final List<String> containerLinks = serviceToStart.getLinks();
+        for (int i = 0;  i < containerLinks.size(); i++) {
+            String link = containerLinks.get(i);


Extracting the body of the for loop in a new method would make it more readable

Taking into account, that we have only one loop in the method and this loop actually is method's body and body of this loop cannot be reused somewhere and if so, we should pass iterator and links list into new method as parameters, IMO, there is no point to extract loop body into another method. @garagatyi WDYT?

Since you are going to refactor this code lets figure out what can be cleaned up after that refactoring

l0rd · 2016-10-25T08:50:14Z

...api-workspace/src/main/java/org/eclipse/che/api/environment/server/CheEnvironmentEngine.java

+    @VisibleForTesting
+    void replaceServiceToContainerNameInLinks(CheServiceImpl serviceToStart, Map<String, CheServiceImpl> services) {
+        final List<String> containerLinks = serviceToStart.getLinks();
+        for (int i = 0;  i < containerLinks.size(); i++) {


You could probably use for each here (for (String link: containersLinks) {)

No, I cannot. I need to change the list. I can use Iterator here, but I think that current version is more readable.

l0rd · 2016-10-25T08:53:43Z

...api-workspace/src/main/java/org/eclipse/che/api/environment/server/CheEnvironmentEngine.java

+            int colonIndex = link.indexOf(':');
+            if (colonIndex != -1) {
+                serviceName = link.substring(0, colonIndex);
+                serviceAlias = link.substring(colonIndex + 1);


What happen if user provide a (bad) link with an empty alias like: serviceDb:

split() method is probably more appropriate here instead of manually parsing the links string.

If user provide link with empty alias, like serviceDb:, than CheEnvironmentValidator will throw an exception and a user will be notified about syntax error.

l0rd · 2016-10-25T09:04:55Z

...api-workspace/src/main/java/org/eclipse/che/api/environment/server/CheEnvironmentEngine.java


+            CheServiceImpl serviceLinkedTo = services.get(serviceName);
+            if (serviceLinkedTo != null) {


An error should be returned if the user is trying to link a service that does not exist (i.e. serviceLinkedTo == null)

I think, that error should be reported. Please note, that things like that are validated by CheEnvironmentValidator.

l0rd · 2016-10-25T09:07:08Z

...api-workspace/src/main/java/org/eclipse/che/api/environment/server/CheEnvironmentEngine.java

+            CheServiceImpl serviceLinkedTo = services.get(serviceName);
+            if (serviceLinkedTo != null) {
+                String containerNameLinkedTo = serviceLinkedTo.getContainerName();
+                containerLinks.set(i, (serviceAlias == null) ?


Instead of modifying current containersLinks list you could make this method return a new list

But in case of no links we need additional logic which made code a bit complicated or return new empty list when it can be untouched, which is a bit weird to me.

l0rd · 2016-10-25T09:09:45Z

...workspace/src/test/java/org/eclipse/che/api/environment/server/CheEnvironmentEngineTest.java

@@ -895,6 +895,43 @@ public void shouldBeAbleToRemoveSnapshot() throws Exception {
        verify(instanceProvider).removeInstanceSnapshot(machineSource);
    }

+    @Test
+    public void shouldReplaceServiceNameWithContainerNameAndAliasInLinks() {


You are testing 2 things here (links with and without alias). You could probably split this test in 2 separate tests.

Signed-off-by: Mykola Morhun <[email protected]>

garagatyi · 2016-10-26T13:37:40Z

...api-workspace/src/main/java/org/eclipse/che/api/environment/server/CheEnvironmentEngine.java

+    void normalizeLinks(CheServiceImpl serviceToNormalizeLinks, Map<String, CheServiceImpl> services) {
+        final List<String> containerLinks = serviceToNormalizeLinks.getLinks();
+        for (int i = 0;  i < containerLinks.size(); i++) {
+            String serviceNameAndAliasToLink[] = containerLinks.get(i).split(":"); // a link has format: 'name:alias' or 'name'


Pass second argument to split

garagatyi · 2016-10-26T13:38:36Z

...api-workspace/src/main/java/org/eclipse/che/api/environment/server/CheEnvironmentEngine.java

+                                      containerNameLinkTo + ':' + serviceAlias);
+            } else {
+                // should never happens. Errors like this should be filtered by CheEnvironmentValidator
+                LOG.error("Attempt to link non existing service {} to {} service.", serviceName, serviceToNormalizeLinks);


It is better to throw IllegalArgumentExcption

garagatyi · 2016-10-26T13:39:37Z

...api-workspace/src/main/java/org/eclipse/che/api/environment/server/CheEnvironmentEngine.java

+            if (serviceLinkTo != null) {
+                String containerNameLinkTo = serviceLinkTo.getContainerName();
+                containerLinks.set(i, (serviceAlias == null) ?
+                                      containerNameLinkTo + ':' + serviceName :


Is it needed? I suppose it already works in such a way?

garagatyi · 2016-10-26T13:41:00Z

...api-workspace/src/main/java/org/eclipse/che/api/environment/server/CheEnvironmentEngine.java

+    @VisibleForTesting
+    void normalizeLinks(CheServiceImpl serviceToNormalizeLinks, Map<String, CheServiceImpl> services) {
+        final List<String> containerLinks = serviceToNormalizeLinks.getLinks();
+        for (int i = 0;  i < containerLinks.size(); i++) {


I would suggest to simplify code with stream API - it looks overcomplicated

garagatyi · 2016-10-26T13:42:47Z

...api-workspace/src/main/java/org/eclipse/che/api/environment/server/CheEnvironmentEngine.java

@@ -667,6 +711,7 @@ private void startEnvironmentQueue(String namespace,
        try {
            machineProvider.createNetwork(networkId);

+            EnvironmentHolder environmentHolder;


Is it still used?

garagatyi · 2016-10-26T13:43:03Z

...api-workspace/src/main/java/org/eclipse/che/api/environment/server/CheEnvironmentEngine.java

@@ -736,7 +781,7 @@ private void startEnvironmentQueue(String namespace,
                boolean queuePolled = false;
                try (StripedLocks.WriteLock lock = stripedLocks.acquireWriteLock(workspaceId)) {
                    ensurePreDestroyIsNotExecuted();
-                    EnvironmentHolder environmentHolder = environments.get(workspaceId);
+                    environmentHolder = environments.get(workspaceId);


Is it still used?

codenvy-ci · 2016-10-26T13:47:50Z

Build # 802 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/802/ to view the results.

Signed-off-by: Mykola Morhun <[email protected]>

garagatyi · 2016-10-27T08:55:23Z

...workspace/src/test/java/org/eclipse/che/api/environment/server/CheEnvironmentEngineTest.java

@@ -895,6 +895,65 @@ public void shouldBeAbleToRemoveSnapshot() throws Exception {
        verify(instanceProvider).removeInstanceSnapshot(machineSource);
    }

+    @Test
+    public void shouldReplaceServiceNameWithContainerNameInLinks() {


Please ensure that code of MachineProviderImpl is also covered

codenvy-ci · 2016-10-27T12:08:45Z

Build # 812 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/812/ to view the results.

Signed-off-by: Mykola Morhun <[email protected]>

mmorhun · 2016-10-28T12:07:11Z

@garagatyi @benoitf @l0rd take a look again please

codenvy-ci · 2016-10-28T12:52:00Z

Build # 829 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/829/ to view the results.

garagatyi

Good job!

l0rd · 2016-10-30T22:40:27Z

Nice @mmorhun! LGTM

Signed-off-by: Mykola Morhun <[email protected]>

codenvy-ci · 2016-10-31T16:53:43Z

Build # 861 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/861/ to view the results.

Mykola Morhun added 3 commits October 13, 2016 11:26

CHE-2687: Add links support in compose synrax

feeb08b

Signed-off-by: Mykola Morhun <[email protected]>

Merge branch 'master' into CHE-2687

322cb2b

Add links support in compose syntax

e764b25

Signed-off-by: Mykola Morhun <[email protected]>

Change some names and comments to more clear

d6dea2c

Signed-off-by: Mykola Morhun <[email protected]>

benoitf reviewed Oct 21, 2016

View reviewed changes

CHE-2687: Add links support in docker compose

709147a

Signed-off-by: Mykola Morhun <[email protected]>

l0rd reviewed Oct 25, 2016

View reviewed changes

Mykola Morhun added 2 commits October 26, 2016 14:01

CHE-2687: Fix broken links section in docker-compose recipe

94814dd

Signed-off-by: Mykola Morhun <[email protected]>

Merge branch 'master' into CHE-2687

b8bee1f

garagatyi suggested changes Oct 26, 2016

View reviewed changes

CHE-2687

052eb76

Signed-off-by: Mykola Morhun <[email protected]>

garagatyi suggested changes Oct 27, 2016

View reviewed changes

Mykola Morhun added 2 commits October 28, 2016 09:17

Merge branch 'master' into CHE-2687

a071f4f

CHE-2687: Add test

0716923

Signed-off-by: Mykola Morhun <[email protected]>

garagatyi approved these changes Oct 28, 2016

View reviewed changes

benoitf approved these changes Oct 31, 2016

View reviewed changes

Mykola Morhun added 2 commits October 31, 2016 16:01

Merge branch 'master' into CHE-2687

7f762c2

CHE-2687: Fix test

255ef67

Signed-off-by: Mykola Morhun <[email protected]>

mmorhun merged commit d74b24f into master Oct 31, 2016

mmorhun deleted the CHE-2687 branch October 31, 2016 14:22

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

Fix broken links section in docker-compose recipe (eclipse-che#2850)

6819484


		CheServiceImpl serviceLinkedTo = services.get(serviceName);
		if (serviceLinkedTo != null) {

Fix broken links section in docker-compose recipe #2850

Fix broken links section in docker-compose recipe #2850

Conversation

mmorhun commented Oct 20, 2016 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Previous behavior

New behavior

mmorhun commented Oct 20, 2016

mmorhun commented Oct 20, 2016

AndrienkoAleksandr commented Oct 20, 2016

codenvy-ci commented Oct 20, 2016

gazarenkov commented Oct 21, 2016

mmorhun commented Oct 21, 2016

benoitf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmorhun Oct 21, 2016 • edited Loading

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

codenvy-ci commented Oct 21, 2016

codenvy-ci commented Oct 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmorhun Oct 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmorhun Oct 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmorhun Oct 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmorhun Oct 26, 2016 • edited Loading

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

codenvy-ci commented Oct 26, 2016

Choose a reason for hiding this comment

codenvy-ci commented Oct 27, 2016

mmorhun commented Oct 28, 2016

codenvy-ci commented Oct 28, 2016

garagatyi left a comment

Choose a reason for hiding this comment

l0rd commented Oct 30, 2016

codenvy-ci commented Oct 31, 2016

mmorhun commented Oct 20, 2016 •

edited

Loading

mmorhun Oct 21, 2016 •

edited

Loading

mmorhun Oct 26, 2016 •

edited

Loading

mmorhun Oct 26, 2016 •

edited

Loading

mmorhun Oct 26, 2016 •

edited

Loading

mmorhun Oct 26, 2016 •

edited

Loading