-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Compose: Make DockerComposeContainer only pull necessary images when multiple compose files were given #3787
Conversation
0f80449
to
306cb3e
Compare
@@ -29,7 +30,7 @@ | |||
private final File composeFile; | |||
|
|||
@Getter | |||
private Set<String> dependencyImageNames = new HashSet<>(); | |||
private Map<String, Set<String>> dependencyImageNames = new HashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wold suggest to name a map as per this convention:
https://stackoverflow.com/questions/2253453/how-should-i-name-a-java-util-map
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know, have changed it to serviceNameToImageNames
Set<String> dependencyImageNames = new HashSet<>(); | ||
|
||
Collection<Set<String>> serviceDependencyImageNames = parsedComposeFiles.stream() | ||
.flatMap(it -> it.getDependencyImageNames().entrySet().stream()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also hard to read this part because you have no idea what dependencyImageNames
is and what does that flatmap
do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reimplemented this part without stream
@@ -76,27 +77,43 @@ public void shouldIgnoreUnknownStructure() { | |||
public void shouldObtainImageNamesV1() { | |||
File file = new File("src/test/resources/docker-compose-imagename-parsing-v1.yml"); | |||
ParsedDockerComposeFile parsedFile = new ParsedDockerComposeFile(file); | |||
assertEquals("all defined images are found", Sets.newHashSet("redis", "mysql", "postgres"), parsedFile.getDependencyImageNames()); // redis, mysql from compose file, postgres from Dockerfile build | |||
Assertions.assertThat(parsedFile.getDependencyImageNames()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting it so that each entry starts at the new line might improve readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formated
…compose files were given
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks perfectly sensible - thanks.
Thanks also to @vcvitaly for the earlier review 👍
Fix #2782