Skip to content
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

Add docker compose support for --build and --rmi options #1625

Merged
merged 16 commits into from Nov 9, 2019
Merged

Conversation

rnorth
Copy link
Member

@rnorth rnorth commented Jul 18, 2019

Replaces #1336
(credit to @sonerd)

Took the changes made here #924 and added test for docker-compose --build

import org.junit.Rule;
import org.testcontainers.containers.DockerComposeContainer;

public class DockerComposeContainerWithBuildTest extends BaseDockerComposeTest {
Copy link
Member

Choose a reason for hiding this comment

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

I am afraid this test does not test it, and only covers the setter. If we break withBuild, the test will continue to pass.

Also, the used compose file (https://github.com/testcontainers/testcontainers-java/blob/151b93ac065a420d0a803d2ebb8ba6d152c783a4/core/src/test/resources/compose-test.yml ) is rather heavy (mysql)

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding the test I was working on a version which tests the created image and so on.
Due to time issues I was not bale to finish it.

I can use a lighter compose file.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've taken this on and am pushing the proposed change now.

/**
* Remove all images used by any service.
*/
all,
Copy link
Member

Choose a reason for hiding this comment

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

perhaps we should use standard naming convention for enums (upper case) and have a field with flag's value?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, can do it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've taken this on and am pushing the proposed change now.

@rnorth rnorth self-assigned this Jul 21, 2019
@rnorth rnorth removed this from the next milestone Jul 21, 2019
@rnorth rnorth added this to the next milestone Aug 12, 2019
@rnorth rnorth modified the milestone: next Sep 4, 2019
@bsideup bsideup modified the milestones: 1.12.3, next Oct 26, 2019
doTest(ALL, false, false);
}

private void doTest(final DockerComposeContainer.RemoveImages removeMode,
Copy link
Member

Choose a reason for hiding this comment

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

@rnorth WDYT about using the parameterized runner instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

May as well - done!

@rnorth rnorth merged commit 055f783 into master Nov 9, 2019
@rnorth rnorth deleted the sonerd-master branch November 9, 2019 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants