Upgrade docker-compose image to latest version and perform dire… #1847
Conversation
…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
| @@ -58,8 +56,7 @@ | |||
| */ | |||
| private final String identifier; | |||
| private final List<File> composeFiles; | |||
| private final Set<String> spawnedContainerIds = new HashSet<>(); | |||
| private final Set<String> spawnedNetworkIds = new HashSet<>(); | |||
There was a problem hiding this comment.
These two sets were never being written to, so they and all code that read them was redundant. Removed.
| @@ -105,10 +102,11 @@ public DockerComposeContainer(String identifier, File... composeFiles) { | |||
| public DockerComposeContainer(String identifier, List<File> composeFiles) { | |||
|
|
|||
| this.composeFiles = composeFiles; | |||
| this.parsedComposeFiles = composeFiles.stream().map(ParsedDockerComposeFile::new).collect(Collectors.toSet()); | |||
There was a problem hiding this comment.
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.
| @@ -169,11 +171,16 @@ public SELF withServices(@NonNull String... services) { | |||
| } | |||
|
|
|||
| private void createServices() { | |||
| // Apply scaling | |||
| final String servicesWithScalingSettings = Stream.concat(services.stream(), scalingPreferences.keySet().stream()) | |||
There was a problem hiding this comment.
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
| @@ -230,72 +233,6 @@ private void runWithCompose(String cmd) { | |||
| .invoke(); | |||
| } | |||
|
|
|||
| @SuppressWarnings("unchecked") | |||
| private static void validate(File composeFile) { | |||
There was a problem hiding this comment.
Extracted to ParsedDockerComposeFile
| } | ||
|
|
||
| spawnedContainerIds.clear(); | ||
| spawnedNetworkIds.clear(); |
There was a problem hiding this comment.
Removed essentially dead code in this section
| return dockerConfig.toString(); | ||
| } else { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
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
| 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); |
There was a problem hiding this comment.
Shouldn't this bee an IllegalArgumentException?
| } | ||
|
|
||
| private void parseAndValidate() { | ||
| if (composeFileContent == null) { |
There was a problem hiding this comment.
Is this necessary?
yaml.load() will return something if the yaml is valid and the other constructor is only available for testing.
There was a problem hiding this comment.
Good point - I think this was a leftover. Removing.
| return; | ||
| } | ||
|
|
||
| Map<String, ?> map = (Map<String, ?>) composeFileContent; |
There was a problem hiding this comment.
IntelliJ shows cast unnecessary.
| return; | ||
| } | ||
|
|
||
| Map<String, ?> map = (Map<String, ?>) composeFileContent; |
There was a problem hiding this comment.
| Map<String, ?> map = (Map<String, ?>) composeFileContent; | |
| Map<String, ?> map = composeFileContent; |
There was a problem hiding this comment.
Why even the local variable? Leftover from a refactoring?
| } | ||
| } | ||
|
|
||
| public Set<String> getServiceImageNames() { |
There was a problem hiding this comment.
Lombok? (the class uses Lombok after all)
| VisibleAssertions.assertThrows("starting with an invalid docker-compose file throws an exception", | ||
| ContainerLaunchException.class, | ||
| IllegalStateException.class, |
There was a problem hiding this comment.
Again I think IllegalArgumentException is better?
| try (DockerComposeContainer container = new DockerComposeContainer<>(file)) { | ||
| Assertions.assertThatThrownBy(container::start) | ||
| Assertions | ||
| .assertThatThrownBy(() -> { |
rnorth
changed the title
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:
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
FROMlayers remain a problem though).