Adding support for Couchbase #269

ldoguin · 2017-01-18T09:35:36Z

This PR adds support for the default Couchbase docker image.

bsideup · 2017-01-18T09:39:20Z

modules/couchbase/src/main/java/org/testcontainers/containers/CouchbaseContainer.java

+
+    @Override
+    protected void configure() {
+        addFixedExposedPort(8091, 8091);


Am I right that these ports are only needed if you use some sub-set of Couchbase APIs?
Fixed ports are dangerous, we were trying to avoid it as much as possible, so maybe we can make it configurable, i.e. "withN1Q(true)" (or whatever uses them), and put a note that it will fix the ports?

I see that tests on Travis are failing because of it as well :(

So, unfortunately, some of the ports cannot be changed. Bad Couchbase design for some services, but not all of them. Out of consistency concerns, I chose to fix everything.
But yes, it could be problematic in many cases.

If consistency isn't an issue, we can change configureable port.

I'm wondering if we can use some free port finder on the host machine and then configure Couchbase with it.

Still flaky, but at least will work for the most of the cases.
WDYT?

bsideup · 2017-01-18T09:41:45Z

modules/couchbase/src/main/java/org/testcontainers/containers/CouchbaseContainer.java

+        BucketSettings bucketSettings = getCouchbaseCluster().clusterManager(clusterUsername, clusterPassword).insertBucket(bucketSetting);
+        // allow some time for the query service to come up
+        try {
+            Thread.sleep(5000);


Can't we somehow continuously check if it's ready?

Could not find an answer yet.

http://docs.couchbase.com/admin/admin/CLI/CBcli/cbcli-bucket-create.html
I see --wait option here - how does it wait?

Also, at least something like Unreliables.retryUntilSuccess(1, TimeUnit.MINUTES, () -> clusterManager.hasBucket(bucketName)) would be better I think

or actually, just clusterManager.hasBucket(bucketName, 1, TimeUnit.MINUTES) :D

bsideup · 2017-01-18T09:42:19Z

modules/couchbase/src/main/java/org/testcontainers/containers/CouchbaseContainer.java

+    }
+
+
+    public void initCluster() {


I think public is too open for this method, maybe protected is better?

actually even private would be better. I intend people to use start anyway, Will fix this.

bsideup · 2017-01-18T09:44:40Z

modules/couchbase/src/main/java/org/testcontainers/containers/CouchbaseContainer.java

+            callCouchbaseRestAPI(poolURL, poolPayload);
+            callCouchbaseRestAPI(setupServicesURL, setupServiceContent);
+            callCouchbaseRestAPI(webSettingsURL, webSettingsContent);
+            callCouchbaseRestAPI(bucketURL, sampleBucketPayloadBuilder.toString());


TestContainers has a lifecycle, you can put this code to containerIsStarting (means that container was created) method, and then, in containerIsStarted (container passed the wait strategy), do

callCouchbaseRestAPI("/settings/indexes", "indexerThreads=0&logLevel=info&maxRollbackPoints=5&storageMode=memory_optimized");

bsideup · 2017-01-18T09:45:58Z

modules/couchbase/src/main/java/org/testcontainers/containers/CouchbaseContainer.java

+        }
+    }
+
+    public void callCouchbaseRestAPI(String url, String payload) throws IOException {


IMO should be protected as well

bsideup · 2017-01-18T09:47:08Z

modules/couchbase/src/main/java/org/testcontainers/containers/CouchbaseContainer.java

+                "application/x-www-form-urlencoded");
+        String encoded = Base64.encode((clusterUsername + ":" + clusterPassword).getBytes("UTF-8"));
+        httpConnection.setRequestProperty("Authorization", "Basic " + encoded);
+        DataOutputStream out = new DataOutputStream(httpConnection.getOutputStream());


add @lombok.Cleanup DataOutputStream out to automatically close the stream (no need to call out.close(), which should be called in try {} finally {})

bsideup · 2017-01-18T09:47:49Z

modules/couchbase/pom.xml

+
+        <dependency>
+            <groupId>${project.groupId}</groupId>
+            <artifactId>selenium</artifactId>


I doubt we need Selenium here :)

bsideup · 2017-01-18T10:25:40Z

modules/couchbase/src/main/java/org/testcontainers/containers/CouchbaseContainer.java

-
-    protected void initCluster() {
+    @Override
+    protected void containerIsStarted(InspectContainerResponse containerInfo) {


I think this code should go to "containerIsStarting", and only the part after waitStrategy - to containerIsStarted. Then you can just define setWaitStrategy() as usual

The ports don't seem to be accessible yet during containerIsStarting.

Ok, so you perform some setups only after wait strategy has passed? Got it 👍

bsideup · 2017-01-18T11:13:06Z

modules/couchbase/pom.xml

+        <dependency>
+            <groupId>com.couchbase.client</groupId>
+            <artifactId>java-client</artifactId>
+            <version>2.4.0</version>


since TestContainers is a library for testing, we probably should use provided scope here, because users will have Couchbase client library already in their app. Right?

bsideup · 2017-01-18T11:13:53Z

modules/couchbase/src/main/java/org/testcontainers/containers/CouchbaseContainer.java

+    private String urlBase;
+
+    public CouchbaseContainer() {
+        super("couchbase/server:latest");


could you please lock the version? We try to avoid ":latest" everywhere. Thanks!

rnorth · 2017-01-22T14:19:24Z

@ldoguin @bsideup how would you feel about aiming to include this in a 1.1.9 release next week? I'm keen to wrap up 1.1.8 and release tonight, as it includes a bunch of bug fixes that people are probably awaiting. 1.1.9 can follow next week.
Please let me know if there's anything I can do to help with this PR, too!
Richard

bsideup · 2017-01-22T14:32:02Z

@rnorth I don't mind! Given a critical hotfix for the Docker 1.1.3 in 1.1.8 we should release it ASAP; everything else we can release as a follow-up release

bsideup · 2017-05-29T11:38:52Z

Hey @ldoguin,

Do you still want to deliver this PR? :)

ldoguin · 2017-05-29T14:14:28Z

I would be happy to but there are some things that are not in my powers. In particular the hardcoded port in Couchbase :(
Maybe @benofben could help?

tchlyah · 2017-07-27T13:18:03Z

Hello @ldoguin,

I've extracted and completed your version in an external module differentway/testcontainers-java-module-couchbase.

@bsideup propose to transfer it under http://github.com/testcontainers, Since I was widely inspired by your version, maybe I need your confirmation?

Thank you.

benofben · 2017-07-28T23:26:21Z

Is there something I can help out with here?

ldoguin · 2017-07-30T00:17:36Z

@tchlyah it looks great! You don't need my permission, please go ahead and do whatever you want with it.

@benofben There were some discussions about Couchbase's fixed port. It would be great if they could all be customized. I believe the Travis tests fail because of this.

And while we are on the customization topic, being able to pass a configuration file as argument to autoconfigure couchbase upon start would be awesome. The asynchronousity of most config changes make it a little bit harder than I would like :) Happy to talk more about this by email :)

benofben · 2017-07-31T21:16:33Z

@ldoguin Are you referring to 8091 or a different port?

ldoguin · 2017-07-31T22:45:33Z

@benofben different ports. All the one that are hardcoded in the CouchbaseContainer case.

    // Non configurable ports
    addFixedExposedPort(8092, 8092);
    addFixedExposedPort(8093, 8093);
    addFixedExposedPort(8094, 8094);
    addFixedExposedPort(8095, 8095);
    addFixedExposedPort(18092, 18092);
    addFixedExposedPort(18093, 18093);

benofben · 2017-07-31T22:58:08Z

I'm not sure I have enough context here to understand. I believe those ports are configurable in the product. It seems like the container isn't exposing that. Maybe mail me at ben.lackey at couchbase.com and you can explain a bit more what you need.

kiview · 2017-08-21T11:41:16Z

Since the module is now developed in it's own repo, it's okay to close the PR, isn't it?

rnorth · 2018-03-13T21:05:41Z

What's the best way to proceed with this. I agree that the existence of differentway/testcontainers-java-module-couchbase means we don't need both.

@tchlyah, would you care to join forces and merge your module into the main testcontainers-java repository? This is an effort we're undertaking for #564.

tchlyah · 2018-03-14T09:30:22Z

@rnorth, yes with pleasure, but I'm in the middle of a move and it's such a mess that I can't find neither time nor my computer in boxes! I will do my best to do this in the next few days. How do you want to proceed?

bsideup · 2018-03-15T10:50:12Z

@rnorth @tchlyah @ldoguin should we close this PR?

bsideup · 2018-05-14T09:36:48Z

Closing in favour of #688

intial commit for couchbase support

178d022

bsideup requested changes Jan 18, 2017

View reviewed changes

bsideup added the enhancement label Jan 18, 2017

bsideup added this to the 1.1.8 milestone Jan 18, 2017

ldoguin added 5 commits January 18, 2017 11:13

remove selenium dependencies

c06aea0

make supported ports dynamic

e388c8d

change init and apiCall method visibility

4d73f54

move to automatic resource cleanup with Lombock

b3e544d

use testcontainers lifecycle methods containerIsStarting

253c00d

bsideup reviewed Jan 18, 2017

View reviewed changes

remove useless wait strategy

45324f3

bsideup reviewed Jan 18, 2017

View reviewed changes

ldoguin added 2 commits January 18, 2017 12:48

use provided scope for the Couchbase SDK

5f8dcfe

use latest stable version

e2a9c05

rnorth modified the milestones: 1.1.9, 1.1.8 Jan 22, 2017

rnorth modified the milestones: 1.1.9, 1.1.10 Feb 12, 2017

bsideup removed this from the 1.1.10 milestone Mar 8, 2017

bsideup mentioned this pull request Jul 27, 2017

Official module tchlyah/testcontainers-java-module-couchbase#1

Open

bsideup added the type/new module label Feb 11, 2018

rnorth mentioned this pull request Mar 13, 2018

Combine Java modules into main testcontainers-java repo #564

Closed

7 tasks

bsideup removed the type/enhancement label Mar 15, 2018

tchlyah mentioned this pull request May 13, 2018

Couchbase module #688

Merged

bsideup closed this May 14, 2018

Adding support for Couchbase #269

Adding support for Couchbase #269

Conversation

ldoguin commented Jan 18, 2017

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

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

rnorth commented Jan 22, 2017

bsideup commented Jan 22, 2017

bsideup commented May 29, 2017

ldoguin commented May 29, 2017

tchlyah commented Jul 27, 2017

benofben commented Jul 28, 2017

ldoguin commented Jul 30, 2017

benofben commented Jul 31, 2017

ldoguin commented Jul 31, 2017

benofben commented Jul 31, 2017

kiview commented Aug 21, 2017

rnorth commented Mar 13, 2018

tchlyah commented Mar 14, 2018

bsideup commented Mar 15, 2018

bsideup commented May 14, 2018