Add Elasticsearch testcontainer #826

dadoonet · 2018-08-10T08:31:44Z

This change moves Elasticsearch testcontainers project from https://github.com/dadoonet/testcontainers-java-module-elasticsearch to testcontainers project so it will benefit from automatic releases and will get more exposure.

dadoonet · 2018-08-10T08:34:02Z

Hey team

This is the first step to move my project under testcontainers umbrella.
I still need to check out the recent changes that happened on Elasticsearch side regarding security.

This is something I might bring within this PR or with another PR.

In the meantime anyway, I'd appreciate a review :)

Thanks!

bsideup · 2018-08-10T08:39:13Z

Hi @dadoonet!

Nice! 👍

I just have one suggestion - could you please split the PR into a few, step by step?
Currently it does a lot of things (ElasticsearchResource, plugins, etc) and it's a bit hard to review. Maybe you can contribute the ElasticsearchContainer first, and other "features" on top of it?

I hope it will not take a lot of your time. Thank you!

dadoonet · 2018-08-10T09:23:07Z

Hmmm. That might take a significant time to do TBH. The full history is there: https://github.com/dadoonet/testcontainers-java-module-elasticsearch/commits/master

I choose to squash it because it was not making a lot of sense to keep the history around. May be that was a bad decision though.

LMK if this is a blocker on your side and I'll do it in the coming month hopefully.

dadoonet · 2018-08-10T12:24:46Z

It took me finally less time than I was expecting. So I might come with something today.

bsideup · 2018-08-10T12:27:37Z

@dadoonet great, thank you!
Sorry for asking, but it will help us a lot to focus on the changes 👍
Looking forward to having an official Elasticsearch module 😍

dadoonet · 2018-08-10T14:55:42Z

@bsideup Done. That was actually interesting to do as it helped me to step back a bit and fix some code, added more tests, etc...

bsideup · 2018-08-12T09:03:42Z

@dadoonet will you submit a PR for each step? It is still a big change to merge at once, would be great to merge them one by one

dadoonet · 2018-08-12T12:26:11Z

Oh? I thought you just wanted to see each commit one by one (which you can do).
I can for sure but isn't that a bit overkill ?

LMK. I might have time in the middle of coming week.

This change moves Elasticsearch testcontainers project from https://github.com/dadoonet/testcontainers-java-module-elasticsearch to testcontainers project so it will benefit from automatic releases and will get more exposure. Step 1 just adds ElasticsearchContainer and its documentation.

dadoonet · 2018-09-21T11:50:51Z

Hey team. So let's just start with some basics. I'll then send new PR to add other features.

kiview · 2018-09-21T11:53:07Z

Oh great, I will give it a look later 🙂

kiview

Thanks for the PR, looking forward to getting this merged. Just had some questions and minor remarks.

docs/SUMMARY.md

docs/usage.md

docs/usage/elasticsearch_container.md

...les/elasticsearch/src/main/java/org/testcontainers/elasticsearch/ElasticsearchContainer.java

...elasticsearch/src/test/java/org/testcontainers/elasticsearch/ElasticsearchContainerTest.java

modules/elasticsearch/build.gradle

...les/elasticsearch/src/main/java/org/testcontainers/elasticsearch/ElasticsearchContainer.java

And fix some bugs. :)

kiview · 2018-09-24T16:38:19Z

Looks like CircleCI is failing for good reasons?
org.testcontainers.containers.ContainerLaunchException: Timed out waiting for URL to be accessible (http://localhost:32774/ should return HTTP 200)
Still I retriggered the build.

dadoonet · 2018-10-09T17:11:39Z

@kiview Could you try instead:

withEnv("discovery.type", "single-node");

Which is something I'd then add by default as for testing I believe this is fine to run one single node.

kiview · 2018-10-09T17:31:45Z

Sounds good, I can check tomorrow.
If you want, just push and we see if CI works

# Conflicts: # circle.yml

kiview · 2018-10-09T17:55:49Z

Awesome, I think we solved it!
I will give it a final review tomorrow.

kiview · 2018-10-15T07:25:23Z

@dadoonet Just wanted to confirm, that it now works on my machine as well.

kiview

I think we have it now, just two tiny remarks. Thanks for putting the effort in this PR, very much appreciated.

kiview · 2018-10-15T07:30:28Z

...les/elasticsearch/src/main/java/org/testcontainers/elasticsearch/ElasticsearchContainer.java

+
+/**
+ * Represents an elasticsearch docker instance which exposes by default port 9200 and 9300 (transport.tcp.port)
+ * The docker image is by default fetch from docker.elastic.co/elasticsearch/elasticsearch


is by default fetched

kiview · 2018-10-15T07:31:25Z

...les/elasticsearch/src/main/java/org/testcontainers/elasticsearch/ElasticsearchContainer.java

+/**
+ * Represents an elasticsearch docker instance which exposes by default port 9200 and 9300 (transport.tcp.port)
+ * The docker image is by default fetch from docker.elastic.co/elasticsearch/elasticsearch
+ * @author dadoonet


Would it be okay for you to omit the author tag? I know we have it in the current codebase, but I think we strive for removing it.

If you would prefer to keep it, keep it 🙂

rnorth · 2018-10-15T07:40:48Z

...elasticsearch/src/test/java/org/testcontainers/elasticsearch/ElasticsearchContainerTest.java

+
+    @Test
+    public void elasticsearchDefaultTest() throws IOException {
+        container = new ElasticsearchContainer();


We can use the try-with-resources pattern here, which means we wouldn't need the stopContainer method (we use it for other tests). No big deal, though.

rnorth · 2018-10-15T07:41:52Z

...les/elasticsearch/src/main/java/org/testcontainers/elasticsearch/ElasticsearchContainer.java

+    /**
+     * Elasticsearch Docker base URL
+     */
+    private static final String ELASTICSEARCH_DEFAULT_BASE_URL = "docker.elastic.co/elasticsearch/elasticsearch";


Could we change the name of this constant to ELASTICSEARCH_DEFAULT_IMAGE or similar?

rnorth · 2018-10-15T07:42:48Z

Thanks for this - looks great to me. I only had two trivial comments but other than that looks good to merge. Cheers!

bsideup · 2018-10-15T07:44:09Z

circle.yml

@@ -31,11 +31,11 @@ jobs:
          when: always
      - store_test_results:
          path: ~/junit
-  modules-no-jdbc-test-no-selenium:
+  modules-no-jdbc-test-no-selenium-no-elasticsearch:


I think we don't need it anymore since we figured out the issue, right?

bsideup · 2018-10-15T07:46:46Z

...les/elasticsearch/src/main/java/org/testcontainers/elasticsearch/ElasticsearchContainer.java

+    public ElasticsearchContainer(String dockerImageName) {
+        super(dockerImageName);
+        logger().info("Starting an elasticsearch container using [{}]", dockerImageName);
+        withNetwork(Network.SHARED);


I would remove it, doesn't make sense unless you connect multiple containers

bsideup · 2018-10-15T08:06:28Z

modules/elasticsearch/src/test/resources/logback-test.xml

+    <logger name="io.netty" level="WARN" />
+    <logger name="org.testcontainers.shaded" level="WARN"/>
+
+    <!-- Prevent noisy logs - any errors thrown will be logged when caught if they need to be -->


not relevant anymore

bsideup · 2018-10-15T08:06:45Z

modules/elasticsearch/src/test/resources/logback-test.xml

+        <appender-ref ref="STDOUT"/>
+    </root>
+
+    <logger name="org.apache.http" level="WARN"/>


you can remove all but org.testcontainers.shaded

dadoonet · 2018-10-27T12:12:33Z

Not sure why travis failed. Could you launch a new build?

kiview · 2018-11-02T09:18:54Z

@dadoonet Of course, sorry. Probably Gradle plugin server was not reachable.
I triggered a restart.

dadoonet · 2018-11-03T13:04:35Z

@kiview Was the build canceled?

kiview · 2018-11-04T15:25:19Z

Thanks for working with us on this @dadoonet, I think we are good now and we can finally merge.

dadoonet requested review from bsideup, kiview and rnorth as code owners August 10, 2018 08:31

bsideup self-assigned this Aug 10, 2018

bsideup added the type/new module label Aug 10, 2018

dadoonet force-pushed the pr/elasticsearch branch from 8b9d7d2 to 7c1a99e Compare August 10, 2018 14:50

dadoonet force-pushed the pr/elasticsearch branch from 7c1a99e to 2a2a5ac Compare September 21, 2018 11:03

kiview requested changes Sep 21, 2018

View reviewed changes

bsideup reviewed Sep 23, 2018

View reviewed changes

modules/elasticsearch/build.gradle Outdated Show resolved Hide resolved

bsideup reviewed Sep 23, 2018

View reviewed changes

...les/elasticsearch/src/main/java/org/testcontainers/elasticsearch/ElasticsearchContainer.java Outdated Show resolved Hide resolved

dadoonet added 5 commits September 24, 2018 11:42

Apply first round of review comments

233372d

Merge branch 'master' into pr/elasticsearch

4cfc315

Remove all properties and use static Strings instead

a19c97e

Do not create a Docker image but just set it

0c1762a

Update to Elasticsearch 6.4.1 (and 5.6.12 in tests)

a93903e

bsideup reviewed Sep 24, 2018

View reviewed changes

...les/elasticsearch/src/main/java/org/testcontainers/elasticsearch/ElasticsearchContainer.java Show resolved Hide resolved

Implement an HttpWaitStrategy and remove withXYZ methods

29d283e

And fix some bugs. :)

Remove License headers

db80613

dadoonet added 2 commits October 9, 2018 18:34

Merge branch 'master' into pr/elasticsearch

f4015f6

# Conflicts: # circle.yml

Force discovery.type to single-node

418adb3

kiview self-assigned this Oct 15, 2018

kiview requested changes Oct 15, 2018

View reviewed changes

rnorth reviewed Oct 15, 2018

View reviewed changes

bsideup reviewed Oct 15, 2018

View reviewed changes

dadoonet added 2 commits October 26, 2018 15:28

Apply changes after review

1c22aed

Fix warnings from codacy

6ac3388

testcontainers deleted a comment Oct 27, 2018

kiview approved these changes Nov 4, 2018

View reviewed changes

kiview added this to the next milestone Nov 4, 2018

kiview merged commit 46b6865 into testcontainers:master Nov 4, 2018

eyalkoren mentioned this pull request Nov 4, 2018

Use official Elasticsearch testcontainer elastic/apm-agent-java#285

Closed

dadoonet deleted the pr/elasticsearch branch November 5, 2018 15:46

ghost mentioned this pull request Jul 9, 2020

ElasticsearchContainer is not waiting for container being ready #2982

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Elasticsearch testcontainer #826

Add Elasticsearch testcontainer #826

dadoonet commented Aug 10, 2018

dadoonet commented Aug 10, 2018

bsideup commented Aug 10, 2018

dadoonet commented Aug 10, 2018

dadoonet commented Aug 10, 2018

bsideup commented Aug 10, 2018

dadoonet commented Aug 10, 2018

bsideup commented Aug 12, 2018

dadoonet commented Aug 12, 2018

dadoonet commented Sep 21, 2018

kiview commented Sep 21, 2018

kiview left a comment

kiview commented Sep 24, 2018

dadoonet commented Oct 9, 2018

kiview commented Oct 9, 2018

kiview commented Oct 9, 2018

kiview commented Oct 15, 2018

kiview left a comment

kiview Oct 15, 2018

kiview Oct 15, 2018

rnorth Oct 15, 2018

rnorth Oct 15, 2018

rnorth commented Oct 15, 2018

bsideup Oct 15, 2018

bsideup Oct 15, 2018

bsideup Oct 15, 2018

bsideup Oct 15, 2018

dadoonet commented Oct 27, 2018

kiview commented Nov 2, 2018

dadoonet commented Nov 3, 2018

kiview commented Nov 4, 2018

Add Elasticsearch testcontainer #826

Add Elasticsearch testcontainer #826

Conversation

dadoonet commented Aug 10, 2018

dadoonet commented Aug 10, 2018

bsideup commented Aug 10, 2018

dadoonet commented Aug 10, 2018

dadoonet commented Aug 10, 2018

bsideup commented Aug 10, 2018

dadoonet commented Aug 10, 2018

bsideup commented Aug 12, 2018

dadoonet commented Aug 12, 2018

dadoonet commented Sep 21, 2018

kiview commented Sep 21, 2018

kiview left a comment

Choose a reason for hiding this comment

kiview commented Sep 24, 2018

dadoonet commented Oct 9, 2018

kiview commented Oct 9, 2018

kiview commented Oct 9, 2018

kiview commented Oct 15, 2018

kiview left a 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

rnorth commented Oct 15, 2018

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

dadoonet commented Oct 27, 2018

kiview commented Nov 2, 2018

dadoonet commented Nov 3, 2018

kiview commented Nov 4, 2018