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.

Sign up for GitHub

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

Jump to bottom

Container definition API #7714

Merged
merged 35 commits into from
Oct 31, 2023
Merged

Container definition API #7714

merged 35 commits into from
Oct 31, 2023

Conversation

eddumelendez
Copy link
Member

Introduce ContainerDef and apply it to PortForwardingContainer
and MongoDBContainer.

@eddumelendez
Copy link
Member Author

We can fix #7708 by switching to this.containerDef.addExposedTcpPorts(ports); here WDYT?

@eddumelendez eddumelendez added this to the next milestone Oct 30, 2023
kiview
kiview reviewed Oct 31, 2023
View reviewed changes
core/src/main/java/org/testcontainers/containers/GenericContainer.java
private List<Bind> binds = new ArrayList<>();

private boolean privilegedMode;

@NonNull
private List<VolumesFrom> volumesFroms = new ArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

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

Will we move it in a follow-up PR to avoid breaking changes?

modules/neo4j/src/test/java/org/testcontainers/containers/Neo4jContainerTest.java Outdated Show resolved Hide resolved
core/src/test/java/org/testcontainers/containers/ExposedHostTest.java Outdated Show resolved Hide resolved
bsideup
bsideup reviewed Oct 31, 2023
View reviewed changes
core/src/main/java/org/testcontainers/containers/GenericContainer.java
Comment on lines -908 to -914
Map<String, String> combinedLabels = new HashMap<>();
combinedLabels.putAll(labels);
if (createCommand.getLabels() != null) {
combinedLabels.putAll(createCommand.getLabels());
}

createCommand.withLabels(combinedLabels);
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change, as previously we were combining the labels

kiview
kiview approved these changes Oct 31, 2023
View reviewed changes
core/src/main/java/org/testcontainers/containers/GenericContainer.java
Comment on lines -908 to -914
Map<String, String> combinedLabels = new HashMap<>();
combinedLabels.putAll(labels);
if (createCommand.getLabels() != null) {
combinedLabels.putAll(createCommand.getLabels());
}

createCommand.withLabels(combinedLabels);
Copy link
Member

Choose a reason for hiding this comment

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

This breaks the order with regards to CreateContainerCmdModifier

Copy link
Member

Choose a reason for hiding this comment

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

Since this is a change to to an undocumented internal behavior, we are fine with accepting it.

@eddumelendez eddumelendez merged commit 1dba8d1 into main Oct 31, 2023
87 checks passed
@eddumelendez eddumelendez deleted the new-container-def-api branch October 31, 2023 16:39
@eddumelendez eddumelendez mentioned this pull request Oct 31, 2023
Closed
@nitesh0420
Copy link

Sadly it broke our tests. 😄

@eddumelendez
Copy link
Member Author

Sorry, but that doesn't provide much information about the issue you are having. We have been reported about some regressions and some of them are fixed now, see this list. There is one more that should be fixed tomorrow. If you can not find yours, please feel free to open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsideup bsideup bsideup left review comments

@kiview kiview kiview approved these changes

Assignees
No one assigned
Labels
modules/mongodb type/enhancement
Projects
None yet
Milestone
1.19.2
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@eddumelendez @nitesh0420 @bsideup @kiview