Skip to content
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

Couchbase module #688

Merged
merged 19 commits into from
Jun 8, 2018
Merged

Couchbase module #688

merged 19 commits into from
Jun 8, 2018

Conversation

tchlyah
Copy link
Contributor

@tchlyah tchlyah commented May 13, 2018

Move Couchbase module to main repo : #564 & #269


### Special consideration

Couchbase testContainer is configured to use random available [ports](https://developer.couchbase.com/documentation/server/current/install/install-ports.html) for some ports only, as [Couchbase Java SDK](https://developer.couchbase.com/documentation/server/current/sdk/java/start-using-sdk.html) permit to configure only some ports :
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI we usually use just word "container"


dependencies {
compile project(':testcontainers')
compile 'com.couchbase.client:java-client:2.5.7'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compileOnly perhaps?

Copy link
Contributor Author

@tchlyah tchlyah May 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couchbase java client is actually used, so if the client doesn't add this library, it won't run.

It is also used in test, it will force me to add testCompile.


/**
* Based on Laurent Doguin version
* <p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JavaDoc task will fail

private String urlBase;

public CouchbaseContainer() {
super("couchbase/server:latest");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please fix the version?


public void callCouchbaseRestAPI(String url, String payload) throws IOException {
String fullUrl = urlBase + url;
HttpURLConnection httpConnection = (HttpURLConnection) ((new URL(fullUrl).openConnection()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please wrap with try-with-resources

* @author ctayeb
* Created on 06/06/2017
*/
public class CouchbaseQueryServiceWaitStrategy extends GenericContainer.AbstractWaitStrategy {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a new base class not based on GenericContainer.AbstractWaitStrategy, could you please use it?

retryUntilSuccess((int) startupTimeout.getSeconds(), TimeUnit.SECONDS, () -> {
getRateLimiter().doWhenReady(() -> {
try {
final HttpURLConnection connection = (HttpURLConnection) new URL(uri).openConnection();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try-with-resources please, otherwise will leak

abstract class AbstractCouchbaseWaitStrategy extends AbstractWaitStrategy {

CouchbaseContainer getContainer() {
return (CouchbaseContainer) waitStrategyTarget;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't have to be an instance of CouchbaseContainer (checked the usages), the same strategy could be used for Docker Compose

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm aware of that, but I didn't find a better solution, I need the container in implemented classes, which was available in the previous implementation of AbstractWaitStrategy. And in my case it is always a CouchbaseContainer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would you need a ref to CouchbaseContainer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To get the container ip address and logger. otherwise I need to add them in constructor.

I'll check this tonight because pushing to github is blocked at work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you get containerIp from WaitStrategyTarget interface.
Logger - I would rather prefer to create it inside the strategy, i.e. with @Slf4j annotation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, will do ASAP. Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@bsideup bsideup self-assigned this May 14, 2018
@bsideup
Copy link
Member

bsideup commented May 16, 2018

Hi @tchlyah,
Could you plz add a line to CHANGELOG.md?

* optimized by Tayeb Chlyah
*/
@AllArgsConstructor
public class CouchbaseContainer<SELF extends CouchbaseContainer<SELF>> extends GenericContainer<SELF> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a final container, you can use:

class CouchbaseContainer extends GenericContainer<CouchbaseContainer>

*
* optimized by Tayeb Chlyah
*/
@AllArgsConstructor
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it really needed?

Copy link
Contributor Author

@tchlyah tchlyah May 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is needed for lombok @Wither

@Override
public void start() {
super.start();
if (!newBuckets.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

containerIsStarted is probably a better method to do this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes indeed. For the record, I never tried to optimize the original code, just make it work 😉

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok, sorry :) Thanks for changing it 👍

@bsideup bsideup added this to the next milestone May 17, 2018
* created on 18/07/16.
*/
@Slf4j
public class CouchbaseWaitStrategy extends AbstractWaitStrategy {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a feeling that this class can be removed or dramatically simplified by using HttpWaitStrategy + responsePredicate :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you are absolutely right, I'm doing it now!

@bsideup bsideup merged commit 3b974e9 into testcontainers:master Jun 8, 2018
@bsideup
Copy link
Member

bsideup commented Jun 8, 2018

@tchlyah merged, thanks! Sorry for taking so long 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants