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

Expose low level API #301

Merged
merged 4 commits into from
Mar 12, 2017
Merged

Expose low level API #301

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9d0b531
Expose low level API. Makes it possible to override unsupported Creat…
bsideup Mar 1, 2017
c4430fe
withCustomizer -> withCreateContainerCmdModifier. Add JavaDoc.
bsideup Mar 3, 2017
66766fd
use LinkedHashSet to keep the order
bsideup Mar 4, 2017
a99a5f9
fix typo
bsideup Mar 10, 2017
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
20 changes: 20 additions & 0 deletions core/src/main/java/org/testcontainers/containers/Container.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,15 +388,35 @@ ExecResult execInContainer(Charset outputCharset, String... command)

void setLinkedContainers(Map<String, LinkableContainer> linkedContainers);

/**
* @deprecated set by GenericContainer and should never be set outside
*/
@Deprecated
Copy link
Member Author

Choose a reason for hiding this comment

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

decided to do the deprecations here

void setDockerClient(DockerClient dockerClient);

/**
* @deprecated set by GenericContainer and should never be set outside
*/
@Deprecated
void setDockerDaemonInfo(Info dockerDaemonInfo);

/**
* @deprecated set by GenericContainer and should never be set outside
*/
@Deprecated
void setContainerId(String containerId);

/**
* @deprecated set by GenericContainer and should never be set outside
*/
@Deprecated
void setContainerName(String containerName);

void setWaitStrategy(WaitStrategy waitStrategy);

/**
* @deprecated set by GenericContainer and should never be set outside
*/
@Deprecated
void setContainerInfo(InspectContainerResponse containerInfo);
}
15 changes: 14 additions & 1 deletion core/src/main/java/org/testcontainers/containers/GenericContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
@EqualsAndHashCode(callSuper = false)
public class GenericContainer<SELF extends GenericContainer<SELF>>
extends FailureDetectingExternalResource
implements Container<SELF> {
implements Container<SELF>, AutoCloseable {
Copy link
Member Author

Choose a reason for hiding this comment

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

small non-related change to simplify the testing

Copy link
Member

Choose a reason for hiding this comment

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

Interesting!

Copy link
Contributor

@vmassol vmassol Jun 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

FTR IntelliJ IDEA is barking about this, see https://imgur.com/a/sD1WNyG ;)

(and I cannot use try with resources in my use case)


private static final Charset UTF8 = Charset.forName("UTF-8");

Expand Down Expand Up @@ -134,6 +134,8 @@ public class GenericContainer<SELF extends GenericContainer<SELF>>

private List<Consumer<OutputFrame>> logConsumers = new ArrayList<>();

private final Set<Consumer<CreateContainerCmd>> createContainerCmdHooks = new HashSet<>();

private static final Set<String> AVAILABLE_IMAGE_NAME_CACHE = new HashSet<>();
private static final RateLimiter DOCKER_CLIENT_RATE_LIMITER = RateLimiterBuilder
.newBuilder()
Expand Down Expand Up @@ -191,6 +193,7 @@ private void tryStart(Profiler profiler) {
profiler.start("Create container");
CreateContainerCmd createCommand = dockerClient.createContainerCmd(dockerImageName);
applyConfiguration(createCommand);
createContainerCmdHooks.forEach(hook -> hook.accept(createCommand));

containerId = createCommand.exec().getId();
ResourceReaper.instance().registerContainerForCleanup(containerId, dockerImageName);
Expand Down Expand Up @@ -881,6 +884,16 @@ public void withStartupAttempts(int attempts) {
this.startupAttempts = attempts;
}

@Override
public void close() {
stop();
}

public SELF withCustomizer(Consumer<CreateContainerCmd> hook) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I need a help with the naming here.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe something more verbose like withPostCreateContainerCmd()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Funny thing - I initially named it withCreateContainerCmdHook and then was playing with different names and accidentally committed this one :D

Copy link
Member

Choose a reason for hiding this comment

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

Like this name too ;)
Although this name does not indicate whether this is a pre- or postcreate hook. Is it important for users of the API to know about this fact? Might be.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've decided to go with "post" hooks only because GenericObject does "pre" part already, and this new API just allows you to configure the final result and override some configuration. Do you have any example where "pre" hook might be helpful? Because I don't :)

Copy link
Member

Choose a reason for hiding this comment

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

No, don't think we need it. My point was only about the naming, including the word post makes the API a bit more self documenting.

Copy link
Member

Choose a reason for hiding this comment

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

I liked pre/post, but actually doesn't it make it confusing wrt. what it is before or after?

This change is post- testcontainer's configuration step, but pre- creation of the container. So actually it seems to be confusing if we're not careful 😂

Some other ideas for the name: withConfigurationCustomizer, withConfigModifier or maybe even just with(Consumer<CreateContainerCmd> hook)..? (The latter would need heavier documentation so as to be more discoverable though)

I'm not sure though 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

Your post clarified the API a lot for me and now I agree, pre/post seems to introduce confusing semantics here.

From a general API naming standpoint, withConfigurationCustomizer and withConfigModifier both sound good, but isn't it actually more a withCreateContainerCmdModifier? Sounds awkward and very verbose, but kind of in line with the docker-java API (which seems appropriate, since docker-java does leak through this method after all).

Copy link
Member

Choose a reason for hiding this comment

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

withCreateContainerCmdModifier seems OK to me - like you say, verbose but clear.

We should probably be making it clear to the developer, through naming and javadoc comments, that this does expose the underlying docker-java API so might change outside of our control.

WDYT @bsideup ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree! I'll add JavaDoc and rename the method now :)

createContainerCmdHooks.add(hook);
return self();
}

/**
* Convenience class with access to non-public members of GenericContainer.
*/
Expand Down
21 changes: 21 additions & 0 deletions core/src/test/java/org/testcontainers/junit/GenericContainerRuleTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import org.rnorth.ducttape.RetryCountExceededException;
import org.rnorth.ducttape.unreliables.Unreliables;
import org.testcontainers.containers.GenericContainer;
import org.testcontainers.utility.Base58;
import org.testcontainers.utility.TestEnvironment;

import java.io.*;
Expand Down Expand Up @@ -282,6 +283,26 @@ public void extraHostTest() throws IOException {
assertTrue("The hosts file of container contains extra host", matcher.find());
}

@Test
public void createContainerCmdHookTest() {
// Use random name to avoid the conflicts between the tests
String randomName = Base58.randomString(5);
try(
Copy link
Member

Choose a reason for hiding this comment

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

👍

GenericContainer container = new GenericContainer<>("redis:3.0.2")
.withCommand("redis-server", "--help")
.withCustomizer(cmd -> cmd.withName("overrideMe"))
// Preserves the order
.withCustomizer(cmd -> cmd.withName(randomName))
// Allows to override pre-configured values by GenericContainer
.withCustomizer(cmd -> cmd.withCmd("redis-server", "--port", "6379"))
) {
container.start();

assertEquals("Name is configured", "/" + randomName, container.getContainerInfo().getName());
assertEquals("Command is configured", "[redis-server, --port, 6379]", Arrays.toString(container.getContainerInfo().getConfig().getCmd()));
}
}

private BufferedReader getReaderForContainerPort80(GenericContainer container) {

return Unreliables.retryUntilSuccess(10, TimeUnit.SECONDS, () -> {
Expand Down