-
-
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
Fail fast if container_name
is set in Docker Compose file
#1581
Conversation
core/src/main/java/org/testcontainers/containers/DockerComposeContainer.java
Show resolved
Hide resolved
core/src/test/java/org/testcontainers/containers/DockerComposeFileValidationTest.java
Show resolved
Hide resolved
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 only have trivial comments, so feel free to respond/dismiss as you see fit, and merge after that.
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 only have trivial comments, so feel free to respond/dismiss as you see fit, and merge after that.
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.
Some suggestions up for discussion.
core/src/test/java/org/testcontainers/containers/DockerComposeFileValidationTest.java
Show resolved
Hide resolved
|
||
@VisibleForTesting | ||
static void validate(Object template, String identifier) { | ||
if (!(template instanceof 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.
I think we could extract 240-261 into a private function and then have
if (!hasKnownFormat(object)) {
log.debug("Compose file {} has an unknown format", identifier);
return
}
...
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.
extracted methods make it much harder to follow the structure IMO, especially when dealing with the format/structure validation
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 disagree, since it makes the responsibility of the part of the code clearer (therefore also increasing maintainability).
But we don't need to fight over this 😉
servicesMap = map; | ||
} | ||
|
||
for (Map.Entry<String, ?> entry : servicesMap.entrySet()) { |
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.
for (Map.Entry<String, ?> entry : servicesMap.entrySet()) {
validateEntry(entry)
}
Just an idea?
We can't break in this case of course.
Fixes #1151. Submitting a new one since #1432 was closed by the author.