Expose low level API #301

bsideup · 2017-03-01T12:06:38Z

Makes it possible to override unsupported CreateContainerCmd properties without waiting until TestContainers adds support for it.

…eContainerCmd properties without waiting until TestContainers adds support for it.

bsideup · 2017-03-01T12:07:06Z

core/src/main/java/org/testcontainers/containers/Container.java

+    /**
+     * @deprecated set by GenericContainer and should never be set outside
+     */
+    @Deprecated


decided to do the deprecations here

bsideup · 2017-03-01T12:07:21Z

core/src/main/java/org/testcontainers/containers/GenericContainer.java

@@ -58,7 +58,7 @@
 @EqualsAndHashCode(callSuper = false)
 public class GenericContainer<SELF extends GenericContainer<SELF>>
        extends FailureDetectingExternalResource
-        implements Container<SELF> {
+        implements Container<SELF>, AutoCloseable {


small non-related change to simplify the testing

Interesting!

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

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

bsideup · 2017-03-01T12:40:05Z

core/src/main/java/org/testcontainers/containers/GenericContainer.java

+        stop();
+    }
+
+    public SELF withCustomizer(Consumer<CreateContainerCmd> hook) {


I need a help with the naming here.

Maybe something more verbose like withPostCreateContainerCmd()?

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

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.

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 :)

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.

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 🤷‍♂️

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).

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 ?

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

martin-g · 2017-03-01T13:21:11Z

What would be the syntax for --tmpfs /var/lib/mysql:rw ?
https://vladmihalcea.com/2017/02/09/how-to-run-integration-tests-at-warp-speed-with-docker-and-tmpfs/

bsideup · 2017-03-01T13:22:28Z

@martin-g I guess we have to wait until tmpfs is supported in docker-java, then it will be something like withCreateContainerCmdHook(cmd -> cmd.withTmpfs())

martin-g · 2017-03-01T13:27:18Z

I see. Thanks!

rnorth · 2017-03-03T08:19:40Z

core/src/main/java/org/testcontainers/containers/GenericContainer.java

@@ -58,7 +58,7 @@
 @EqualsAndHashCode(callSuper = false)
 public class GenericContainer<SELF extends GenericContainer<SELF>>
        extends FailureDetectingExternalResource
-        implements Container<SELF> {
+        implements Container<SELF>, AutoCloseable {


Interesting!

rnorth · 2017-03-03T08:20:45Z

core/src/test/java/org/testcontainers/junit/GenericContainerRuleTest.java

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


rnorth · 2017-03-10T15:16:18Z

core/src/main/java/org/testcontainers/containers/GenericContainer.java

@@ -134,6 +134,8 @@

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

+    private final Set<Consumer<CreateContainerCmd>> createContainerCmdMidifiers = new LinkedHashSet<>();


Spelling: Midifiers -> Modifiers

rnorth · 2017-03-10T15:18:07Z

Aside from a spelling error this LGTM!

I think this warrants separate mention in the docs, though. I can try and do this at the weekend if you don't have time though - up to you.

## [1.2.0] - 2017-03-12 ### Fixed - Fix various escaping issues that may arise when paths contain spaces (#263, #279) - General documentation fixes/improvements (#300, #303, #304) - Improve reliability of `ResourceReaper` when there are a large number of containers returned by `docker ps -a` (#295) ### Changed - Support Docker for Windows via TCP socket connection (#291, #297, #309). _Note that Docker Compose is not yet supported under Docker for Windows (see #306) - Expose `docker-java`'s `CreateContainerCmd` API for low-level container tweaking (#301) - Shade `org.newsclub` and Guava dependencies (#299, #292) - Add `org.testcontainers` label to all containers created by Testcontainers (#294)

Expose low level API. Makes it possible to override unsupported Creat…

9d0b531

…eContainerCmd properties without waiting until TestContainers adds support for it.

bsideup commented Mar 1, 2017

View reviewed changes

bsideup requested a review from rnorth March 1, 2017 12:07

bsideup self-assigned this Mar 1, 2017

bsideup added the enhancement label Mar 1, 2017

bsideup added this to the 1.1.10 milestone Mar 1, 2017

bsideup mentioned this pull request Mar 1, 2017

Is there a way to set the memory capacity for a GenericContainer? #298

Closed

bsideup commented Mar 1, 2017

View reviewed changes

rnorth reviewed Mar 3, 2017

View reviewed changes

bsideup added 2 commits March 3, 2017 11:01

withCustomizer -> withCreateContainerCmdModifier. Add JavaDoc.

c4430fe

use LinkedHashSet to keep the order

66766fd

bsideup mentioned this pull request Mar 10, 2017

Start container with hostname #311

Closed

rnorth reviewed Mar 10, 2017

View reviewed changes

fix typo

a99a5f9

rnorth merged commit 570b4e8 into master Mar 12, 2017

rnorth deleted the low_level_api branch March 12, 2017 20:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose low level API #301

Expose low level API #301

bsideup commented Mar 1, 2017

bsideup Mar 1, 2017

bsideup Mar 1, 2017

rnorth Mar 3, 2017

vmassol Jun 27, 2018 •

edited

Loading

bsideup Mar 1, 2017

kiview Mar 1, 2017

bsideup Mar 1, 2017

kiview Mar 1, 2017

bsideup Mar 1, 2017

kiview Mar 1, 2017

rnorth Mar 1, 2017

kiview Mar 1, 2017

rnorth Mar 3, 2017

bsideup Mar 3, 2017

martin-g commented Mar 1, 2017

bsideup commented Mar 1, 2017

martin-g commented Mar 1, 2017

rnorth Mar 3, 2017

rnorth Mar 3, 2017

rnorth Mar 10, 2017

rnorth commented Mar 10, 2017

		@@ -134,6 +134,8 @@

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

		private final Set<Consumer<CreateContainerCmd>> createContainerCmdMidifiers = new LinkedHashSet<>();

Expose low level API #301

Expose low level API #301

Conversation

bsideup commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmassol Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-g commented Mar 1, 2017

bsideup commented Mar 1, 2017

martin-g commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnorth commented Mar 10, 2017

vmassol Jun 27, 2018 •

edited

Loading