-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
Rewrite Couchbase module. closes #2447 #2491
Conversation
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.
* 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we add the original source of this file, for future reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment is invalid ;)
public static final String VERSION = "5.5.1"; | ||
public static final String DOCKER_IMAGE_NAME = "couchbase/server:"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is a big breaking change after all, but in other modules, we sometimes also have image name and tag public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see #2054 for the discussion
private final CouchbaseEnvironment couchbaseEnvironment = createCouchbaseEnvironment(); | ||
private static final OkHttpClient HTTP_CLIENT = new OkHttpClient() | ||
.newBuilder() | ||
.readTimeout(Duration.ofMinutes(1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big timeout necessary for startup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are per-strategy timeouts too, I see no harm here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Show resolved
Hide resolved
/** | ||
* Checks if already running and if so raises an exception to prevent too-late setters. | ||
*/ | ||
private void checkNotRunning() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea from a user perspective, but this behaves very different (better?) than other TC modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we also check isRunning()
from getMappedPort
, for example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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...
.forPath("/pools/default") | ||
.forPort(MGMT_PORT) | ||
.withBasicCredentials(username, password) | ||
.forStatusCode(200) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be omitted, we use 200 as default
.forPath("/admin/ping") | ||
.forPort(QUERY_PORT) | ||
.withBasicCredentials(username, password) | ||
.forStatusCode(200) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does knowing about status code 200 help?
.forPath("/pools/default/buckets/" + bucket.getName()) | ||
.forPort(MGMT_PORT) | ||
.withBasicCredentials(username, password) | ||
.forStatusCode(200) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above
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!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
.bootstrapCarrierDirectPort(container.getMappedPort(11210)) | ||
.bootstrapHttpDirectPort(container.getMappedPort(8091)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I was wondering if it wouldn't be nicer to make those default ports public constants in the container class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
.forPath("/admin/ping") | ||
.forPort(QUERY_PORT) | ||
.withBasicCredentials(username, password) | ||
.forStatusCode(200) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does knowing about status code 200 help?
modules/couchbase/src/main/java/org/testcontainers/couchbase/CouchbaseContainer.java
Outdated
Show resolved
Hide resolved
modules/couchbase/src/main/java/org/testcontainers/couchbase/CouchbaseContainer.java
Outdated
Show resolved
Hide resolved
modules/couchbase/src/main/java/org/testcontainers/couchbase/CouchbaseContainer.java
Outdated
Show resolved
Hide resolved
Co-Authored-By: Kevin Wittek <kiview@users.noreply.github.com>
Docs are now incorrect 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
…tainers#2491)" This reverts commit c6e0cd9.
…tainers#2491)" This reverts commit c6e0cd9.
Closes #2486.