Skip to content

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

Closed
mnafshin wants to merge 3 commits into
testcontainers:masterfrom
mnafshin:master
Closed

mnafshin wants to merge 3 commits into
testcontainers:masterfrom
mnafshin:master

Conversation

@mnafshin

Copy link
Copy Markdown
Contributor

…list of them is provided via withServices(...).

…list of them is provided via withServices(...).
runWithCompose("up " + flags);
} else {
runWithCompose("up " + flags + " " + servicesWithScalingSettings);
runWithCompose("up " + flags + " " + servicesWithScalingSettings + " " + String.join(" ", this.services));

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 •

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 •

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.

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 😅

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

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

@ekis

ekis commented Jun 19, 2020

Copy link
Copy Markdown

This is a really important part of functionality for people using DockerComposeContainer that is currently broken - can we continue to review this PR and have this eventually merged?

@ekis ekis left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Built locally with these changes and observed the fix - formerly, all containers from local docker-compose.yml would be started up. Now, only containers matching services start.

@rnorth

rnorth commented Jun 22, 2020

Copy link
Copy Markdown
Member

Closing, as #2922 replaces this one.

@rnorth rnorth closed this Jun 22, 2020
@rnorth

rnorth commented Jun 24, 2020

Copy link
Copy Markdown
Member

FYI, we've merged #2922 and will release soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants