Upgrade docker-compose image to latest version and perform dire… by rnorth · Pull Request #1847 · testcontainers/testcontainers-java

rnorth · 2019-09-09T17:17:49Z

Together with using Compose file 2.1 syntax, this is a solution to network cleanup issue described in:

Solution to general credential helper authenticated pull issues in:

Compose build doesn't respect credential helpers docker/compose#5854

Tangentially should add support for v3 syntax (not yet tested) re #531

Key points to be aware of

The containerised version we were using (1.8.0) is ancient and only supports compose file v2.0 syntax. To respond to Networks created in docker-compose file are not labeled with com.docker.compose.project docker/compose#6636, we need to upgrade, so I have moved us to the latest release version
A knock-on effect is that newer versions of compose understand the concept of credential helpers, but cannot use them while dockerized. So instead, I've moved the image pull step so that it is performed using our docker-java client, which supports authenticated pulls with credential helpers.
A nice side effect that I would have liked to do anyway is sidestepping Compose build doesn't respect credential helpers docker/compose#5854, which makes it difficult to use authenticated images in compose files. We're now immune to this problem for images that are used directly in the compose file (Dockerfile builds with authenticated FROM layers remain a problem though).

…ge pull Together with using Compose file 2.1 syntax, this is a solution to network cleanup issue described in: * #1767 * #739 * testcontainers/moby-ryuk#2 * docker/compose#6636 Solution to general credential helper authenticated pull issues in: * docker/compose#5854 Tangentially should add support for v3 syntax (not yet tested) re #531

rnorth · 2019-09-09T17:18:26Z

    private final String identifier;
    private final List<File> composeFiles;
-    private final Set<String> spawnedContainerIds = new HashSet<>();
-    private final Set<String> spawnedNetworkIds = new HashSet<>();


These two sets were never being written to, so they and all code that read them was redundant. Removed.

rnorth · 2019-09-09T17:25:18Z

    public DockerComposeContainer(String identifier, List<File> composeFiles) {

        this.composeFiles = composeFiles;
+        this.parsedComposeFiles = composeFiles.stream().map(ParsedDockerComposeFile::new).collect(Collectors.toSet());


Perform light parsing of the docker compose files at instantiation. We could delay this if it's felt doing this is too heavy at instantiation-time, but if we do that we just need to make sure we do it at some point.

rnorth · 2019-09-09T17:26:44Z


    private void createServices() {
+        // Apply scaling
+        final String servicesWithScalingSettings = Stream.concat(services.stream(), scalingPreferences.keySet().stream())


Scaling behaviour changed in newer docker-compose; rather than old behaviour of:

docker-compose scale someservice=2 docker-compose up -d

we now have to do:

docker-compose up -d --scale someservice=2

rnorth · 2019-09-09T17:27:17Z

    }

-    @SuppressWarnings("unchecked")
-    private static void validate(File composeFile) {


Extracted to ParsedDockerComposeFile

rnorth · 2019-09-09T17:27:36Z

-                }
-
-                spawnedContainerIds.clear();
-                spawnedNetworkIds.clear();


Removed essentially dead code in this section

rnorth · 2019-09-09T17:28:45Z

-            return dockerConfig.toString();
-        } else {
-            return null;
-        }


Exposing docker config.json to the containerised compose now does more harm than good, as now understands, but can't use, the credential helpers.

Instead, I removed this config passthrough altogether.

…stcontainers-java into upgrade-docker-compose

kiview

Some small remarks.

kiview · 2019-10-13T10:47:35Z

+        try (FileInputStream fileInputStream = FileUtils.openInputStream(composeFile)) {
+            composeFileContent = yaml.load(fileInputStream);
+        } catch (Exception e) {
+            throw new IllegalStateException("Unable to parse YAML file from " + composeFile.getAbsolutePath(), e);


Shouldn't this bee an IllegalArgumentException?

Fair point, will change.

kiview · 2019-10-13T11:17:50Z

+    }
+
+    private void parseAndValidate() {
+        if (composeFileContent == null) {


Is this necessary?
yaml.load() will return something if the yaml is valid and the other constructor is only available for testing.

Good point - I think this was a leftover. Removing.

kiview · 2019-10-13T11:19:41Z

+            return;
+        }
+
+        Map<String, ?> map = (Map<String, ?>) composeFileContent;


IntelliJ shows cast unnecessary.

kiview · 2019-10-13T11:19:59Z

+            return;
+        }
+
+        Map<String, ?> map = (Map<String, ?>) composeFileContent;


Suggested change

Map<String, ?> map = (Map<String, ?>) composeFileContent;

Map<String, ?> map = composeFileContent;

Why even the local variable? Leftover from a refactoring?

kiview · 2019-10-13T11:24:28Z

+        }
+    }
+
+    public Set<String> getServiceImageNames() {


Lombok? (the class uses Lombok after all)

kiview · 2019-10-13T11:27:01Z

-
        VisibleAssertions.assertThrows("starting with an invalid docker-compose file throws an exception",
-                ContainerLaunchException.class,
+            IllegalStateException.class,


Again I think IllegalArgumentException is better?

All comments should be addressed

bsideup · 2019-11-12T21:22:49Z

-        try (DockerComposeContainer container = new DockerComposeContainer<>(file)) {
-            Assertions.assertThatThrownBy(container::start)
+        Assertions
+            .assertThatThrownBy(() -> {


nit: some indentation issue

bsideup

We're entering a dangerous field of maintaining the compose files parser but since there are no other (good) option, I guess that's something we can do to dramatically improve the experience with Docker Compose, and this is something we should do 👍

rnorth commented Sep 9, 2019

View reviewed changes

rnorth and others added 8 commits September 10, 2019 11:49

Catch pull-related exceptions

297acba

Merge branch 'master' into upgrade-docker-compose

72838a6

Merge branch 'master' into upgrade-docker-compose

90b7a7c

Merge branch 'master' into upgrade-docker-compose

f6447a8

Merge branch 'master' into upgrade-docker-compose

16d9f48

Merge branch 'master' into upgrade-docker-compose

31792ca

Merge branch 'upgrade-docker-compose' of github.com:testcontainers/te…

219ab2b

…stcontainers-java into upgrade-docker-compose

Merge branch 'master' into upgrade-docker-compose

ab84dda

rnorth marked this pull request as ready for review October 12, 2019 16:43

rnorth requested review from bsideup and kiview as code owners October 12, 2019 16:43

kiview previously requested changes Oct 13, 2019

View reviewed changes

Address review comments

e941dac

rnorth requested a review from kiview October 13, 2019 15:53

rnorth self-assigned this Nov 1, 2019

Merge master into upgrade-docker-compose

721e020

bsideup reviewed Nov 12, 2019

View reviewed changes

bsideup approved these changes Nov 12, 2019

View reviewed changes

bsideup added this to the next milestone Nov 12, 2019

bsideup added area/docker-compose type/feature labels Nov 12, 2019

Merge branch 'master' into upgrade-docker-compose

1cbce4d

rnorth added 3 commits November 22, 2019 21:09

Fix merge error

165754f

Fix indentation

d8b3edc

Merge branch 'master' into upgrade-docker-compose

4fb573d

rnorth changed the title ~~Upgrade docker-compose image to latest version and perform direct image pull~~ Upgrade docker-compose image to latest version and perform dire… Nov 23, 2019

rnorth merged commit b756527 into master Nov 23, 2019

rnorth deleted the upgrade-docker-compose branch November 23, 2019 16:40

This was referenced Dec 7, 2019

Docker-compose v3 format is not supported #531

Closed

Question: Should containers/networks be pruned after failed tests? #739

Open

E4Edwards mentioned this pull request Dec 17, 2019

DockerComposeContainer not starting up specific services #2191

Closed

	Map<String, ?> map = (Map<String, ?>) composeFileContent;
	Map<String, ?> map = composeFileContent;

Conversation

rnorth commented Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Key points to be aware of

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rnorth Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rnorth Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiview left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsideup left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rnorth commented Sep 9, 2019 •

edited

Loading

rnorth Sep 9, 2019 •

edited

Loading

rnorth Sep 9, 2019 •

edited

Loading