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.

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

Allow testcontainers to have custom labels #725

Merged
merged 6 commits into from
Jun 4, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
25 changes: 25 additions & 0 deletions core/src/main/java/org/testcontainers/containers/Container.java
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,15 @@ public String getStderr() {
*/
void addEnv(String key, String value);

/**
* Add a label to the container. Consider using {@link #withLabel(String, String)}
* for build a container in a fluent style.
*
* @param key label key
* @param value label value
*/
void addLabel(String key, String value);
Copy link
Member

Choose a reason for hiding this comment

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

should we avoid having non-fluent version of such methods? Or is there any value it adds?
/cc @rnorth @kiview

Copy link
Member

Choose a reason for hiding this comment

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

I would be okay with deprecating the old ones and not adding new ones.


/**
* Adds a file system binding. Consider using {@link #withFileSystemBind(String, String, BindMode)}
* for building a container in a fluent style.
Expand Down Expand Up @@ -204,6 +213,22 @@ default SELF withEnv(String key, Function<Optional<String>, String> mapper) {
*/
SELF withEnv(Map<String, String> env);

/**
* Add a label to the container.
*
* @param key label key
* @param value label value
* @return this
*/
SELF withLabel(String key, String value);

/**
* Add labels to the container.
* @param labels map of labels
* @return this
*/
SELF withLabels(Map<String, String> labels);

/**
* Set the command that should be run in the container
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,9 @@ public class GenericContainer<SELF extends GenericContainer<SELF>>
@NonNull
private Map<String, String> env = new HashMap<>();

@NonNull
private Map<String, String> labels = new HashMap<>();

@NonNull
private String[] commandParts = new String[0];

Expand Down Expand Up @@ -473,6 +476,7 @@ private void applyConfiguration(CreateContainerCmd createCommand) {
Map<String, String> labels = createCommand.getLabels();
labels = new HashMap<>(labels != null ? labels : Collections.emptyMap());
labels.putAll(DockerClientFactory.DEFAULT_LABELS);
labels.putAll(this.labels);
Copy link
Member

Choose a reason for hiding this comment

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

I would do it before DockerClientFactory.DEFAULT_LABELS (they must be set to the values we provide), and also before createContainerCmdModifiers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I a developer really wants to overwrite the default labels he or she should be able to do so.
I tried overwriting the org.testcontainers label with false and left the session_id untouched - the containers still were removed.

Copy link
Member

Choose a reason for hiding this comment

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

sorry, IMO this is too dangerous.
Also, why would anyone override internal labels of Testcontainers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok - should we silently ignore org.testcontainers.* labels or throw an exception?

Copy link
Member

Choose a reason for hiding this comment

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

an exception, plus an order change ( createContainerCmdModifiers are supposed to be low level and should be able to override any pre-set values)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the university is closing down - I'll change this tomorrow.

Copy link
Member

Choose a reason for hiding this comment

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

Sure! Thanks for your contribution :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bsideup done :) Please review all the changes, I will squash the commits when you give your okay ;)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! No need to squash, GitHub allows us to do it automatically 🎉

createCommand.withLabels(labels);
}

Expand Down Expand Up @@ -580,6 +584,14 @@ public void addEnv(String key, String value) {
env.put(key, value);
}

/**
* {@inheritDoc}
*/
@Override
public void addLabel(String key, String value) {
labels.put(key, value);
}

/**
* {@inheritDoc}
*/
Expand Down Expand Up @@ -700,6 +712,24 @@ public SELF withEnv(Map<String, String> env) {
return self();
}

/**
* {@inheritDoc}
*/
@Override
public SELF withLabel(String key, String value) {
this.addLabel(key, value);
return self();
}

/**
* {@inheritDoc}
*/
@Override
public SELF withLabels(Map<String, String> labels) {
labels.forEach(this::addLabel);
return self();
}

/**
* {@inheritDoc}
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.net.Socket;
import java.time.Duration;
import java.util.Arrays;
import java.util.Map;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import java.util.regex.Matcher;
Expand Down Expand Up @@ -127,6 +128,15 @@ public static void setupContent() throws FileNotFoundException {
.withExtraHost("somehost", "192.168.1.10")
.withCommand("/bin/sh", "-c", "while true; do cat /etc/hosts | nc -l -p 80; done");

/**
* Create a container with a custom label for testing.
*/
@ClassRule
public static GenericContainer alpineCustomLabel = new GenericContainer("alpine:3.2")
Copy link
Member

@bsideup bsideup May 31, 2018

Choose a reason for hiding this comment

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

please use try-with-resources, otherwise this container will be started for any test of this class

Copy link
Member

Choose a reason for hiding this comment

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

Also, the container for this test can be as simple as:

new GenericContainer()
    .withLabel("our.custom", "label")
    .withCommand("ps")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it behaves like every other container in this test class ;)

Copy link
Member

Choose a reason for hiding this comment

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

not every, rather legacy ;)

Copy link
Member

Choose a reason for hiding this comment

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

See createContainerCmdHookTest, copyToContainerTest and others

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see - much better :)

.withExposedPorts(80)
.withLabel("our.custom", "label")
.withCommand("/bin/sh", "-c", "while true; do cat /etc/hosts | nc -l -p 80; done");

// @Test
// public void simpleRedisTest() {
// String ipAddress = redis.getContainerIpAddress();
Expand Down Expand Up @@ -220,6 +230,14 @@ public void environmentFromMapTest() throws IOException {
assertEquals("Environment variables can be passed into a command from a map", "42 and 50", line);
}

@Test
public void customLabelTest() {
Map<String, String> labels = alpineCustomLabel.getCurrentContainerInfo().getConfig().getLabels();
assertTrue("org.testcontainers label is present", labels.containsKey("org.testcontainers"));
assertTrue("our.custom label is present", labels.containsKey("our.custom"));
assertEquals("our.custom label value is label", labels.get("our.custom"), "label");
}

@Test
public void customClasspathResourceMappingTest() throws IOException {
// Note: This functionality doesn't work if you are running your build inside a Docker container;
Expand Down Expand Up @@ -341,7 +359,7 @@ public void copyToContainerTest() throws Exception {

try (final GenericContainer alpineCopyToContainer = new GenericContainer("alpine:3.2")
.withCommand("top")){

alpineCopyToContainer.start();
final MountableFile mountableFile = MountableFile.forClasspathResource("test_copy_to_container.txt");
alpineCopyToContainer.copyFileToContainer(mountableFile, "/home/");
Expand Down
7 changes: 5 additions & 2 deletions docs/usage/generic_containers.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,15 @@ public static GenericContainer redis =

// Set up a plain OS container and customize environment,
// command and exposed ports. This just listens on port 80
// and always returns '42'
// and always returns '42'.
// The container will have a custom label named 'our.custom'
// with a value 'label'.
@ClassRule
public static GenericContainer alpine =
new GenericContainer("alpine:3.2")
.withExposedPorts(80)
.withEnv("MAGIC_NUMBER", "42")
.withLabel("our.custom", "label")
Copy link
Member

Choose a reason for hiding this comment

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

I think this use case (labels) is way too specific and should be documented separately (this examples shows common usage of the GenericContainer)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, fair enough

.withCommand("/bin/sh", "-c",
"while true; do echo \"$MAGIC_NUMBER\" | nc -l -p 80; done");
```
Expand All @@ -46,4 +49,4 @@ The class rule provides methods for discovering how your tests can interact with
For example, with the Redis example above, the following will allow your tests to access the Redis service:
```java
String redisUrl = redis.getContainerIpAddress() + ":" + redis.getMappedPort(6379);
```
```
8 changes: 8 additions & 0 deletions docs/usage/options.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,14 @@ new GenericContainer(...)
.withEnv("API_TOKEN", "foo")
```

### Labels

To add a custom label to the container, use `withLabel`:
```java
new GenericContainer(...)
.withLabel("your.custom", "label")
```

### Command

By default the container will execute whatever command is specified in the image's Dockerfile. To override this, and specify a different command, use `withCommand`:
Expand Down