-
-
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
CouchbaseContainer fixes #830
CouchbaseContainer fixes #830
Conversation
will rebase when #829 is merged. |
@@ -67,12 +61,11 @@ | |||
@AllArgsConstructor | |||
public class CouchbaseContainer extends GenericContainer<CouchbaseContainer> { | |||
|
|||
public static final String VERSION = "5.1.0"; | |||
public static final String VERSION = "5.1.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.
any reason we stick to 5.1.1 here? 5.5.0 is out. Is there a reason it is not using latest if not otherwise specified explicitly by the user?
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.
api compatibility. But this release will have breaking changes anyway for couchbase containers, so why not
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.
👍
@@ -392,6 +358,11 @@ public CouchbaseContainer withIndexMemoryQuota(String indexMemoryQuota) { | |||
return self(); | |||
} | |||
|
|||
public CouchbaseContainer withClusterUsername(String clusterUsername) { |
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 wonder if this should be withAdminUsername / withAdminPassword ? to distinguish it from real RBAC users
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 like the word cluster for this context. it allows me to manage the whole thing.
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.
what about ClusterAdmin ? makes both the scope and the role clear.
@@ -1,94 +0,0 @@ | |||
package org.testcontainers.containers; |
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.
is this an unrelated change?
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 a lot of changes coming from @bsideup's pr
d410297
to
5574a89
Compare
- removed example buckets - added withClusterAdmin method - fixed capi port mapping: the environment patching was removed from the couchbase docker container. couchbase/docker@86e665b#diff-8941c93c7af57eff48058be15eb5ecff - updated container from 5.1.0 to 5.5.1
5574a89
to
835bbd3
Compare
We have this out in a Release Candidate build (1.9.0-rc1) for anyone who is keen to try it! |
the environment patching was removed from the couchbase docker container.
couchbase/docker@86e665b#diff-8941c93c7af57eff48058be15eb5ecff