Skip to content

Fix 2191. Is is not running all services of a docker-compose, when a … #2250

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.

Sign up for GitHub

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

Closed
mnafshin wants to merge 3 commits into from
Closed

Fix 2191. Is is not running all services of a docker-compose, when a … #2250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
70bce69
Fix 2191. Is is not running all services of a docker-compose, when a …
mnafshin Jan 14, 2020
e0427fc
Fix 2191. removed not useful extend of BaseDockerComposeTest for Dock…
mnafshin Jan 14, 2020
c486f56
Fix 2191. missing test condition for making sure only those services …
mnafshin Jan 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions core/src/main/java/org/testcontainers/containers/DockerComposeContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ private void createServices() {
if(Strings.isNullOrEmpty(servicesWithScalingSettings)) {
runWithCompose("up " + flags);
} else {
runWithCompose("up " + flags + " " + servicesWithScalingSettings);
runWithCompose("up " + flags + " " + servicesWithScalingSettings + " " + String.join(" ", this.services));

@bsideup bsideup Apr 11, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

uh? servicesWithScalingSettings already includes services: Stream.concat(services.stream(), ...

@mnafshin mnafshin Apr 11, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think there is a misunderstanding of how scaling works in docker-compose
if you want to scale some services you must specify their scaling exactly like what is done in servicesWithScalingSettings. For example, it would be something like:
docker-compose up --scale redis=2
the misunderstanding is here: the previous command is do nothing different as the
docker-compose up
that brings up all services (only redis will have 2 instances)

But, if you want to bring up only some of the services (not all) one must specify the name of services. For example:
docker-compose up --scale redis=2 redis
this command will only brings up 2 instance of redis
or
docker-compose up --scale redis=2 redis db
this command will set up 2 instances of redis and 1 instance of db

@rnorth rnorth Jun 22, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Damn, I've been looking at this for ages and every time failing to understand the issue.

The current implementation fails to limit the running services. My misunderstanding was thinking that it was failing to launch any containers which are in withServices but not in withScaledServices.

Actually that is and always has been fine, but we're getting services launched that we do no want.

e.g. with a docker-compose.yml file like:

redis:
  image: redis
db:
  image: orchardup/mysql
  environment:
    MYSQL_DATABASE: wordpress

and

    public DockerComposeContainer environment = new DockerComposeContainer(new File("src/test/resources/compose-test.yml"))
        .withServices("redis");

(taken from the test)

... we'd expect on redis to come up, but actually db is launched too.

The same goes for:

    public DockerComposeContainer environment = new DockerComposeContainer(new File("src/test/resources/compose-test.yml"))
        .withServices("redis") // with or without this line
        .withScaledService("redis", 2);

... we get 2 redis, plus one db that wasn't asked for.

We should absolutely fix this.

@mnafshin thanks for the PR. I'd just like to add to the tests to ensure that we have coverage for:

  • withServices is used (this is your existing test)
  • withServices and withScaledService are both used together
  • withScaledService is used on its own

If I can push to your branch I will do so.

@rnorth rnorth Jun 22, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm glad I extended the test cases to be more comprehensive, because I've discovered some more ways this can break with different combinations of inputs.

Slightly bigger commit incoming 😅

@rnorth rnorth Jun 22, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It seems that I don't have permission to push to @mnafshin's fork, so I've created a new PR: #2922

@mnafshin mnafshin Jun 23, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If you want I can add you, but I think it is a little late, and you have made your own repo

}
}

Expand Down Expand Up @@ -253,7 +253,7 @@ private void registerContainersForShutdown() {
));
}

private List<Container> listChildContainers() {
public List<Container> listChildContainers() {
return dockerClient.listContainersCmd()
.withShowAll(true)
.exec().stream()
Expand Down
40 changes: 40 additions & 0 deletions core/src/test/java/org/testcontainers/junit/DockerComposeContainerWithServicesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package org.testcontainers.junit;

import com.github.dockerjava.api.model.Container;
import org.junit.Rule;
import org.junit.Test;
import org.testcontainers.containers.DockerComposeContainer;

import java.io.File;
import java.util.HashSet;
import java.util.List;
import java.util.Set;

import static org.rnorth.visibleassertions.VisibleAssertions.assertEquals;


public class DockerComposeContainerWithServicesTest {

private static final String[] listOfServices = new String[]{"redis"/*, "db"*/};

@Rule
public DockerComposeContainer environment = new DockerComposeContainer(new File("src/test/resources/compose-test.yml"))
.withServices(listOfServices);

@Test
public void testGivenDockerCompose_WhenSublistOfServicesSelected_ThenOnlyThoseServicesAreStarted() {
Set<String> runningList = new HashSet<>();

List<Container> list = environment.listChildContainers();
for (Container container : list) {
for (String service : listOfServices)
if (container.getNames()[0].contains(service)) {
runningList.add(service);
}
}
assertEquals("number of running services of docker-compose is the same as length of listOfServices",
listOfServices.length, environment.listChildContainers().size());
assertEquals("Running services are the same as the requested listOfServices",
listOfServices.length, runningList.size());
}
}