Skip to content

Check for a label alongside of the server version #25

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

Merged
lanwen merged 5 commits into from
Feb 18, 2025
Merged

Check for a label alongside of the server version #25

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
769b48e
DRC-30 - Check for a label instead of server version
realjenius Feb 14, 2025
6b90c21
Follow the label semantics
lanwen Feb 17, 2025
4e08ccd
Fix label
lanwen Feb 17, 2025
748cfd4
Better wording
lanwen Feb 17, 2025
06c9ac1
label check to format message
lanwen Feb 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/test/java/cloud/testcontainers/example/TestcontainersCloudFirstTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.util.Arrays;

import com.github.dockerjava.api.DockerClient;
import com.github.dockerjava.api.model.Info;
Expand Down Expand Up @@ -39,12 +40,19 @@ public void testcontainersCloudDockerEngine() {
Info dockerInfo = client.infoCmd().exec();

String serverVersion = dockerInfo.getServerVersion();
assertThat(serverVersion)
String[] labels = dockerInfo.getLabels();

boolean isCloudServer = labels != null && Arrays.asList(labels).contains("docker/cloud");

if (!isCloudServer) {
Copy link
Member

@lanwen lanwen Feb 14, 2025

Choose a reason for hiding this comment

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

It's generally a bad practice to have ifs in tests. Perhaps, we can design it as multiple tests and just skip those not matching preconditions?

Copy link
Contributor Author

@realjenius realjenius Feb 14, 2025

Choose a reason for hiding this comment

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

It was mostly an intermediate state situation while we are rolling the change out. But yes, I'm open to suggestions.

Copy link
Member

@lanwen lanwen Feb 14, 2025

Choose a reason for hiding this comment

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

Even for the intermediate step, it can be confusing. Also, it shares a bad message about our ways of doing tests this way. I'd vote for having separate tests for TCD connection and another for TCC at the end. But I need to experiment a bit with a zone that has the label first

assertThat(serverVersion)
.as("Docker Client is configured via the Testcontainers desktop app")
.satisfiesAnyOf(
dockerString -> assertThat(dockerString).contains("Testcontainers Desktop"),
dockerString -> assertThat(dockerString).contains("testcontainerscloud")
Copy link
Member

@lanwen lanwen Feb 14, 2025

Choose a reason for hiding this comment

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

That shouldn't be possible anymore, right?

Copy link
Contributor Author

@realjenius realjenius Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Well, I was coding it to support a gradual rollout, since we probably want to update the examples before we ship the new change.

But yes, once rolled out, it should just be assert that the docker cloud label exists.

Copy link
Member

@lanwen lanwen Feb 14, 2025

Choose a reason for hiding this comment

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

satisfiesAnyOf will hit Testcontainers Desktop all the time, as we didn't remove that part from TCD, so it makes no sense even for a gradual rollout.

);
);
}


String runtimeName = "Testcontainers Cloud";
if (!serverVersion.contains("testcontainerscloud")) {
Expand Down