Rewrite Couchbase module. closes #2447 #2491

bsideup · 2020-03-29T17:00:26Z

Closes #2486.

This changeset completely reworks the couchbase module and hopefully greatly improve the out-of-the-box experience. Note that this is a breaking change over the previous code because by intention it does NOT depend on SDK 2 so you can test SDK 2 and 3 with it at the same time. Highlights: - Removed the need for a SDK, so both 2 and 3 can be used with it. - Updated to 6.5.0 baseline and using alternate addresses for "proper" port exposure without having to rely on the socat proxy container like the previous version had to. - Allows to define which services should be exposed and handles states automatically (i.e. will not try to create the primary index if the query service is not enabled). Note that a bunch of tests have been removed since they are not adequate anymore. A side effect of the alternate address change is that older servers cannot be used. 6.5.0 is available in both CE and EE, and Couchbase in general allows EE versions to be used in development and testing so we should use it if we can.

kiview · 2020-03-29T17:58:21Z

modules/couchbase/src/main/java/org/testcontainers/couchbase/BucketDefinition.java

+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.


Ideally, we add the original source of this file, for future reference.

I think this comment is invalid ;)

kiview · 2020-03-29T18:08:52Z

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

-    public static final String VERSION = "5.5.1";
-    public static final String DOCKER_IMAGE_NAME = "couchbase/server:";


I know this is a big breaking change after all, but in other modules, we sometimes also have image name and tag public.

see #2054 for the discussion

kiview · 2020-03-29T18:09:30Z

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

-    private final CouchbaseEnvironment couchbaseEnvironment = createCouchbaseEnvironment();
+    private static final OkHttpClient HTTP_CLIENT = new OkHttpClient()
+        .newBuilder()
+        .readTimeout(Duration.ofMinutes(1))


Big timeout necessary for startup?

there are per-strategy timeouts too, I see no harm here

We can reduce it too if needed, but I figured maybe on slower machines it might help not break

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

kiview · 2020-03-29T18:12:41Z

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

+    /**
+     * Checks if already running and if so raises an exception to prevent too-late setters.
+     */
+    private void checkNotRunning() {


Nice idea from a user perspective, but this behaves very different (better?) than other TC modules.

we also check isRunning() from getMappedPort, for example

@bsideup funny: that's how I ended it up adding it like this, because I accidentally had to get a mapped port before it was running...

kiview · 2020-03-29T18:29:18Z

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

+                .forPath("/pools/default")
+                .forPort(MGMT_PORT)
+                .withBasicCredentials(username, password)
+                .forStatusCode(200)


could be omitted, we use 200 as default

kiview · 2020-03-29T18:29:27Z

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

+                    .forPath("/admin/ping")
+                    .forPort(QUERY_PORT)
+                    .withBasicCredentials(username, password)
+                    .forStatusCode(200)


Let's not :D
Really does not help, especially when someone who knows Couchbase wants to see which endpoint is used to check the liveness here

Why does knowing about status code 200 help?

kiview · 2020-03-29T18:31:11Z

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

+                .forPath("/pools/default/buckets/" + bucket.getName())
+                .forPort(MGMT_PORT)
+                .withBasicCredentials(username, password)
+                .forStatusCode(200)


kiview · 2020-03-29T18:39:54Z

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

+        final Map<String, ContainerNetwork> networks = getContainerInfo().getNetworkSettings().getNetworks();
+        for (ContainerNetwork network : networks.values()) {
+            return network.getIpAddress();
+        }
+        throw new IllegalStateException("No network available to extract the internal IP from!");


Suggested change

final Map<String, ContainerNetwork> networks = getContainerInfo().getNetworkSettings().getNetworks();

for (ContainerNetwork network : networks.values()) {

return network.getIpAddress();

}

throw new IllegalStateException("No network available to extract the internal IP from!");

return getContainerInfo().getNetworkSettings().getNetworks().values().stream()

.map(ContainerNetwork::getIpAddress)

.findFirst()

.orElseThrow(() -> new IllegalStateException("No network available to extract the internal IP from!"));

Not sure if better, but on first reading, I stumbled upon the for-loop for getting the first element.

kiview · 2020-03-29T18:44:06Z

modules/couchbase/src/test/java/org/testcontainers/couchbase/CouchbaseContainerTest.java

+                .bootstrapCarrierDirectPort(container.getMappedPort(11210))
+                .bootstrapHttpDirectPort(container.getMappedPort(8091))


Here I was wondering if it wouldn't be nicer to make those default ports public constants in the container class?

@kiview one other option would be to provide explicit getters for only those two that you might need for bootstrapping? So people don't have to concern themselves with the implementation details at all. This is needed for SDK 2 only, for SDK 3 all you need to do is getConnectionString() .. so something like getCarrierDirectPort() and getHttpDirectPort() and the same for the ssl ports if someone wants to use that.

kiview · 2020-04-12T17:28:43Z

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

+                    .forPath("/admin/ping")
+                    .forPort(QUERY_PORT)
+                    .withBasicCredentials(username, password)
+                    .forStatusCode(200)


Why does knowing about status code 200 help?

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

Co-Authored-By: Kevin Wittek <kiview@users.noreply.github.com>

rnorth · 2020-04-12T18:07:21Z

Docs are now incorrect 😅

rnorth

Looks great! Thanks for all your hard work on this @bsideup and @daschl!

rnorth · 2020-04-13T19:47:48Z

This was released in https://github.com/testcontainers/testcontainers-java/releases/tag/1.14.0 🎉

Thanks for the contribution!

…2491) * Rewrite couchbase module. closes testcontainers#2447 This changeset completely reworks the couchbase module and hopefully greatly improve the out-of-the-box experience. Note that this is a breaking change over the previous code because by intention it does NOT depend on SDK 2 so you can test SDK 2 and 3 with it at the same time. Highlights: - Removed the need for a SDK, so both 2 and 3 can be used with it. - Updated to 6.5.0 baseline and using alternate addresses for "proper" port exposure without having to rely on the socat proxy container like the previous version had to. - Allows to define which services should be exposed and handles states automatically (i.e. will not try to create the primary index if the query service is not enabled). Note that a bunch of tests have been removed since they are not adequate anymore. A side effect of the alternate address change is that older servers cannot be used. 6.5.0 is available in both CE and EE, and Couchbase in general allows EE versions to be used in development and testing so we should use it if we can. * Wait until query respsonds with 200 * fixes * add `getBootstrap*DirectPort()` methods, review fixes * Restore `getConnectionString()` * Apply suggestions from code review Co-Authored-By: Kevin Wittek <kiview@users.noreply.github.com> * Update docs * Update docs (2) Co-authored-by: Michael Nitschinger <michael@nitschinger.at> Co-authored-by: Kevin Wittek <kiview@users.noreply.github.com>

…tainers#2491)" This reverts commit c6e0cd9.

daschl and others added 3 commits March 27, 2020 16:06

Wait until query respsonds with 200

7613933

fixes

e02eab9

bsideup added type/breaking-api-change Public APIs are being changed modules/couchbase labels Mar 29, 2020

bsideup added this to the next milestone Mar 29, 2020

bsideup requested a review from a team March 29, 2020 17:00

bsideup requested review from kiview and rnorth as code owners March 29, 2020 17:00

bsideup mentioned this pull request Mar 29, 2020

Rewrite couchbase module. closes #2447 #2486

Closed

kiview approved these changes Mar 29, 2020

View reviewed changes

This was referenced Apr 3, 2020

Fix Couchbase FTS support #2350

Closed

Include Testcontainers in the main generator jhipster/generator-jhipster#11544

Closed

pascalgrimaud mentioned this pull request Apr 5, 2020

Couchbase: disabled hipster-labs/jhipster-daily-builds#46

Closed

bsideup and others added 3 commits April 12, 2020 15:19

add getBootstrap*DirectPort() methods, review fixes

c81e7bf

Merge branch 'master' into daschl/couchbase-rewrite

c1e2273

Restore getConnectionString()

8d1cf17

kiview approved these changes Apr 12, 2020

View reviewed changes

Apply suggestions from code review

81def1d

Co-Authored-By: Kevin Wittek <kiview@users.noreply.github.com>

bsideup added 2 commits April 12, 2020 20:12

Update docs

10f9e9d

Update docs (2)

067f4d8

rnorth approved these changes Apr 12, 2020

View reviewed changes

bsideup merged commit c6e0cd9 into master Apr 12, 2020

delete-merged-branch bot deleted the daschl/couchbase-rewrite branch April 12, 2020 18:23

bsideup added the type/feature label Apr 12, 2020

sd-yip added a commit to sd-yip/testcontainers-java that referenced this pull request May 11, 2021

Revert "Rewrite Couchbase module. closes testcontainers#2447 (testcon…

4c10f5d

…tainers#2491)" This reverts commit c6e0cd9.

sd-yip added a commit to sd-yip/testcontainers-java that referenced this pull request May 12, 2021

Revert "Rewrite Couchbase module. closes testcontainers#2447 (testcon…

d80c94c

…tainers#2491)" This reverts commit c6e0cd9.

sd-yip added a commit to sd-yip/testcontainers-java that referenced this pull request May 12, 2021

Revert "Rewrite Couchbase module. closes testcontainers#2447 (testcon…

5549c54

…tainers#2491)" This reverts commit c6e0cd9.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite Couchbase module. closes #2447 #2491

Rewrite Couchbase module. closes #2447 #2491

bsideup commented Mar 29, 2020 •

edited by rnorth

kiview Mar 29, 2020

kiview Mar 29, 2020

kiview Mar 29, 2020

bsideup Mar 29, 2020

kiview Mar 29, 2020

bsideup Mar 29, 2020

daschl Mar 30, 2020

kiview Mar 29, 2020

bsideup Mar 29, 2020

daschl Mar 30, 2020

kiview Mar 29, 2020

kiview Mar 29, 2020

bsideup Mar 29, 2020

kiview Apr 12, 2020

kiview Mar 29, 2020

kiview Mar 29, 2020

kiview Mar 29, 2020

daschl Mar 30, 2020

kiview Apr 12, 2020

rnorth commented Apr 12, 2020

rnorth left a comment

rnorth commented Apr 13, 2020

		public static final String VERSION = "5.5.1";
		public static final String DOCKER_IMAGE_NAME = "couchbase/server:";

		.bootstrapCarrierDirectPort(container.getMappedPort(11210))
		.bootstrapHttpDirectPort(container.getMappedPort(8091))

Rewrite Couchbase module. closes #2447 #2491

Rewrite Couchbase module. closes #2447 #2491

Conversation

bsideup commented Mar 29, 2020 • edited by rnorth

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 Apr 12, 2020

rnorth left a comment

Choose a reason for hiding this comment

rnorth commented Apr 13, 2020

bsideup commented Mar 29, 2020 •

edited by rnorth