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

Implement Cassandra support #525

Merged
merged 6 commits into from
Jul 10, 2018

Conversation

AnkBurov
Copy link
Contributor

@AnkBurov AnkBurov commented Dec 17, 2017

PR to implement separate Cassandra container with keyspace initialization support (and any other CQL statements of course). I've also refactored a bit ScriptUtils class by implementing database delegate abstraction - it allows Cassandra container reuse existing code in a kinda neat way to execute CQL statements without massive code copypasting.
Datastax Cassandra driver is used due to it's the only Cassandra client Java driver that has suitable license, 3.x Casandra version support, is reliable and has many stars on Github. 😆

Resolve #523

@kiview
Copy link
Member

kiview commented Dec 20, 2017

It would be fine with me to revert to using Lambdas (I would even prefer them). Somehow I don't see the corresponding Codacy issue.

<dependencies>
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>jdbc</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

Cassandra (and your implementation) does not provide a JDBC support at all, we should remove the dependency on JDBC module.
AbstractDatabaseDelegate seems to be over-engineered as well

Copy link
Contributor Author

@AnkBurov AnkBurov Dec 22, 2017

Choose a reason for hiding this comment

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

Good point. I separated Script engine from SQL implementation, but didn't do from modules perspective. I created new commons module that contains ScriptUtils (which is not SQL-tied up and can run any scripts) and DatabaseDelegate interface with its abstract child. Implementations of the delegate lie in corresponding modules.
AbstractDatabaseDelegate remove excess try with resources.
Cassandra module now also depends only on commons module.

Copy link
Member

@bsideup bsideup left a comment

Choose a reason for hiding this comment

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

I would not change JDBC module in this PR. They have almost nothing in common.

protected void configure() {
optionallyMapResourceParameterAsVolume(CONTAINER_CONFIG_LOCATION, configLocation);
addExposedPort(CQL_PORT);
setStartupAttempts(3);
Copy link
Member

Choose a reason for hiding this comment

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

please move it to the constructor, so that it will be overridable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks!
Also many containers have this line of code and it just overwrites any value that could be set through withStartupAttempts method. Think it's undesirable behavior.

private String initScriptPath;

public CassandraContainer() {
super(IMAGE + ":latest");
Copy link
Member

Choose a reason for hiding this comment

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

can be replaced with this(IMAGE + ":latest") to perform additional configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks!

@@ -0,0 +1,1233 @@
# Cassandra storage config YAML
Copy link
Member

Choose a reason for hiding this comment

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

IMO we should remove the commented options to make it easier to understand what config we use to run C* by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default Cassandra container does nothing with Cassandra config and doesn't have any default config. It just uses config from Docker container (unlike of Maria db testcontainer) as is. This file is only example of configuration. Btw, I added comment line there to clear things out.

Copy link
Member

Choose a reason for hiding this comment

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

If it's unused, we definitely should not put it in src/main/resources.
If user decides to override the config he most probably knows its structure and examples

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I said - it's only an example. As mentioned in top of the file and also in packet naming.

probably

It's only a guess. I believe that more information is better. Ok, if you really want, I removed this packet completely.

CassandraContainer cassandraContainer = (CassandraContainer) new CassandraContainer("cassandra:" + cassandraVersion)
.withLogConsumer(new Slf4jLogConsumer(logger));
cassandraContainer.start();
try {
Copy link
Member

Choose a reason for hiding this comment

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

you can use try-with-resources

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't know that GenericContainer implements AutoClosable. Thanks! Btw, I reverted back for lambda usage so the commented line is not an issue anymore.

}

@Test
public void testConfigurationOverride() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

this test does not test the override actually but rather demonstrates the issue with current implementation :D
If user provided a configuration and it's not found, we should fail instead of starting the container with our own, default config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! 👍
Replaced by select of the changed cluster_name property from overridden configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also added test that container won't start if user specified configuration directory with empty content.


@Override
public void execute(Collection<String> statements, String scriptPath, boolean continueOnError, boolean ignoreFailedDrops) {
try (DatabaseDelegate closeableDelegate = this) {
Copy link
Member

Choose a reason for hiding this comment

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

why do we close our-self after "execute" call? O_O

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 close the connection after the statements execution. But it's a bad practice. Removed.

@AnkBurov
Copy link
Contributor Author

@bsideup thanks for the review! 👍 I'l see the remarks in the next few days.

@AnkBurov
Copy link
Contributor Author

AnkBurov commented Dec 22, 2017

Fixed the remarks, see answers to comments above.
In short: script engine separated from SQL implementation. I created new commons module that contains ScriptUtils (which is not SQL-tied up and can run any scripts) and DatabaseDelegate interface with its abstract child. Implementations of the delegate lie in corresponding modules.

this(dockerImageName, 3);
}

public CassandraContainer(String dockerImageName, int startupAttempts) {
Copy link
Member

Choose a reason for hiding this comment

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

Since we have a fluent version withStartupAttempts, I don't think this constructor is useful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@Override
protected void configure() {
optionallyMapResourceParameterAsVolume(CONTAINER_CONFIG_LOCATION, configLocation);
addExposedPort(CQL_PORT);
Copy link
Member

Choose a reason for hiding this comment

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

please also move to the constructor. The purpose of configure method is to allow configuring some parameters provided as fluent calls i.e. configLocation, but any other "static" parameter should go to constructors to allow overriding them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

private void runInitScriptIfRequired() {
if (initScriptPath != null) {
try {
URL resource = Optional.ofNullable(Thread.currentThread().getContextClassLoader().getResource(initScriptPath))
Copy link
Member

Choose a reason for hiding this comment

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

You don't need ofNullable here.

URL resource = Thread.currentThread().getContextClassLoader().getResource(initScriptPath);
if (resource == null) {
    logger().warn("Could not load classpath init script: {}", initScriptPath);
    throw new ScriptLoadException("Could not load classpath init script: " + initScriptPath + ". Resource not found.")
}

Copy link
Contributor Author

@AnkBurov AnkBurov Dec 24, 2017

Choose a reason for hiding this comment

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

I believe that Optional is just better (for readability) than ancient simple null check. Replaced by null check.
Also now this try block is similar to block in ContainerDatabaseDriver and can be extracted to ScriptUtils (since it's not bound to concrete database implementation).

@Override
public void execute(String statement, String scriptPath, int lineNumber, boolean continueOnError, boolean ignoreFailedDrops) {
try {
ResultSet result = getConnection().execute(statement);
Copy link
Member

Choose a reason for hiding this comment

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

at least in JDBC world you must close the result set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Override
public void closeConnectionQuietly() {
try {
getConnection().getCluster().close();
Copy link
Member

Choose a reason for hiding this comment

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

new CassandraDatabaseDelegate().closeConnectionQuietly() will create a connection and then close it. Should check if connection is created and do nothing instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Added null check before closing connection. Also changed visibility of closeConnectionQuietly() method.

@@ -0,0 +1,1233 @@
# Cassandra storage config YAML
Copy link
Member

Choose a reason for hiding this comment

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

If it's unused, we definitely should not put it in src/main/resources.
If user decides to override the config he most probably knows its structure and examples

public void testSimple() throws Exception {
CassandraContainer cassandraContainer = (CassandraContainer) new CassandraContainer()
.withLogConsumer(new Slf4jLogConsumer(logger));
performWithContainer(cassandraContainer, () -> {
Copy link
Member

Choose a reason for hiding this comment

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

why not just (as in the other tests):

try(CassandraContainer<?> container = new CassandraContainer<>()) {
  container.start();
  // Do something with container
}

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before writing tests I saw SimpleMySQLTest. Btw personally I don't like try with resources if class uses builder initialization - it becomes a little clumsy and hard to read. That's why I used lambdas.
Ok, lambdas replaced by proposed try-with-resources style.

@@ -0,0 +1,23 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member

Choose a reason for hiding this comment

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

Bad naming. commons is too generic, while this module only provides some DB script utils

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced by database-commons.

@@ -0,0 +1,23 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
Copy link
Member

Choose a reason for hiding this comment

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

I checked the code of this module. Sorry if I didn't make it clear last time, but:

  1. I still don't see the value of sharing it between Cassandra and JDBC-based containers.
  2. If you really want to do this change, I would rather split this PR into two, one change at a time.
  3. previously mentioned naming issues

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, maybe I didn't clear out enough why I did this in the first place.

I brought DatabaseDelegate approach not just because I wanted to, but because Cassandra script initialization requires the same code that lies in ScriptUtils. ScriptUtils is currently tied up with SQL and JDBC, but in real life database scripts can be different languages and use different databases, and JDBC is not always the answer. Just like with CQL.
If I didn't bring new abstraction, then I would be forced to duplicate massive amount of code and only change java.sql.Connection argument to com.datastax.driver.core.Session. This code duplication is not just bad - it's terrible.

So I untied script parsing and execution from concrete database implementation by bringing DatabaseDelegate abstraction.

@@ -212,17 +210,16 @@ public static boolean containsSqlScriptDelimiters(String script, String delim) {
return false;
}

public static void executeSqlScript(Connection connection, String scriptPath, String script) throws ScriptException {
Copy link
Member

Choose a reason for hiding this comment

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

By doing that, you break the backward compatibility because the method was public and users might use it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really don't think that somebody uses ScriptUtils from testcontainers library, so it's not even a problem from my perspective.
And btw this class is intended for internal usage:

Mainly for internal use within the framework.

@AnkBurov
Copy link
Contributor Author

@bsideup @kiview @rnorth any response?

@kiview
Copy link
Member

kiview commented Jan 19, 2018

Hey @AnkBurov, sorry for not getting in touch with you.
Do you think you can keep the changes of this PR to the cassandra module and add the database abstraction code in another PR?

<version>3.3.2</version>
</dependency>

<!--Netty dependencies for docker tests-->
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why these are needed, they should transitive dependencies after all? I'm not a big Maven expert, so maybe someone else can explain this?

Copy link
Contributor Author

@AnkBurov AnkBurov Feb 4, 2018

Choose a reason for hiding this comment

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

@kiview Datastax Cassandra driver has its own Netty dependency: https://github.com/datastax/java-driver/blob/57724bd63c3038e3728260c7640abcea06f33b86/driver-core/pom.xml#L38
which overrides transitive Netty dependency from com.github.docker-java.

Netty from Datastax driver has 4.0.47 driver
Netty from com.github.docker-java (which is testcontainers core dependency) has 4.1.11.Final version.
So it all leads to behavior, that in Cassandra tests Docker container cannot be started due to absence of io.netty.channel.ChannelFactory (4.0.47 has this interface in different io.netty.bootstrap package).

To resolve this jar purgatory, I use explicit Netty dependency for Cassandra module tests with version same as in com.github.docker-java.
In production (import jar testcontainers core jar and new cassandra jar) there is no com.github.docker-java dependency, so there is no problem with different Netty versions, so it's only transitive Netty from Datastax, which works fine with everything.

Datastax has Jira ticket to update Netty version, but it will only be in 4.x version, which is not available yet: https://datastax-oss.atlassian.net/browse/JAVA-1241

@AnkBurov
Copy link
Contributor Author

Hello @kiview

Do you think you can keep the changes of this PR to the cassandra module and add the database abstraction code in another PR?

The only way to do that is to do a massive code duplication by creating CassandraScriptUtils which will contain all ScriptUtils code with the only difference of Cassandra-specific database connection instead of java.sql.Connection. It's awful solution, so I didn't do that from the start and generalized ScriptUtils to work with any database, not just relational as it is right now.
So yeah, it can be done, but it wouldn't be the code which I'm proud of :)

@bsideup
Copy link
Member

bsideup commented Jan 19, 2018

a massive code duplication

I would argue about "massive" ;)

So yeah, it can be done, but it wouldn't be the code which I'm proud of :)

Then I would recommend to create a first PR where you generalize script utils and second where you add Cassandra support :)

@AnkBurov
Copy link
Contributor Author

AnkBurov commented Jan 19, 2018

@bsideup more than two duplicated LoC is by default massive :)

Okay, I'll split this PR into parts and create separate PR with database delegate abstraction and generalized ScriptUtils from this PR.

rnorth pushed a commit that referenced this pull request Feb 3, 2018
rnorth pushed a commit that referenced this pull request Feb 3, 2018
Abstracted and changed database init script functionality to support use of SQL-like scripts with non-JDBC connections.

Supports #525
@AnkBurov
Copy link
Contributor Author

AnkBurov commented Feb 4, 2018

Okay, database delegate abstraction and generalized ScriptUtils are merged, therefore this PR contains only new Cassandra container related code (and few new exceptions from ScriptUtils - discussable btw). I also squashed commits.

@bsideup @kiview please take a look. Thanks!


private CassandraContainer container;

public CassandraDatabaseDelegate(CassandraContainer container) {
Copy link
Member

Choose a reason for hiding this comment

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

please mark container as final and add @RequiredArgsConstructor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I forgot about Lombok. Done, thanks! 👍

throw new ScriptUtils.ScriptStatementFailedException(statement, lineNumber, scriptPath);
}
} catch (DriverException e) {
throw new ScriptUtils.ScriptStatementFailedException(statement, lineNumber, scriptPath, e);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should import ScriptUtils.ScriptStatementFailedException?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added static import.

try {
return Cluster.builder()
.addContactPoint(container.getContainerIpAddress())
.withPort(container.getMappedPort(CQL_PORT))
Copy link
Member

Choose a reason for hiding this comment

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

WDYT about adding cassandraContainer.getCluster() method? So that consumers will use something like:

return cassandraContainer.getCluster().newSession();

And if somebody extends the C* class and change the CQL port, it will still work

Copy link
Contributor Author

@AnkBurov AnkBurov Feb 8, 2018

Choose a reason for hiding this comment

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

That's actually a really good idea. I used to initialize Datastax datasource in my projects using system variables, but this allows to don't create them at all, if Cassandra is needed only in tests and not in the main configuration.
So it's a good alternative to system variables in tests. Done, thanks!


@Test
public void testSimple() throws Exception {
try (CassandraContainer cassandraContainer = (CassandraContainer) new CassandraContainer()
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why do you cast it to (CassandraContainer)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GenericContainer.withLogConsumer() makes returning type GenericContainer as well (something with these "embedded" generics). So to be able use CassandraContainer-specific methods I need to cast it.

See existing SimpleMySQLTest - same approach.

Copy link
Member

Choose a reason for hiding this comment

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

Just use new CassandraContainer<>() :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, diamond operator. Sure, generic container has generics. Done, thanks!

private static class ScriptParseException extends RuntimeException {
public ScriptParseException(String format, String scriptPath) {
super(String.format(format, scriptPath));
}
}

public static class ScriptStatementFailedException extends RuntimeException {
public ScriptStatementFailedException(String statement, int lineNumber, String scriptPath) {
Copy link
Member

Choose a reason for hiding this comment

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

indentation error. Also, could you please call this(statement, lineNumber, scriptPath, null) and then call different supers if ex is null or not, so that String.format code will be shared (Didn't you say you hate code duplication? ;) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indentation error.

Intellij Idea shows that's everything is ok 😺
screenshot from 2018-02-08 16-12-21

Also, could you please call this(statement, lineNumber, scriptPath, null) and then call different supers if ex is null or not, so that String.format code will be shared

Good idea, done, thanks!

(Didn't you say you hate code duplication? ;) )

I did, but I don't like passing nulls into not mine methods :) But currently no NPE here, so I've done it.

/**
* Map (effectively replace) directory in Docker with the content of resourceLocation if resource location is not null
*
* Protected just in case of overridden behavior
Copy link
Member

Choose a reason for hiding this comment

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

Can you please rephrase this to sound less ambigious? Maybe

Protected to alllow for changing implementation by extending the class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks.

private static final String TEST_CLUSTER_NAME_IN_CONF = "Test Cluster Integration Test";

@Test
public void testSimple() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

All throws Exception declarations are unecessary in this test class.

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're right. It's just my Intellij Idea template for test methods.
Need to change it :)

…tCluster() method, remove excess exception usage

@Test(expected = ContainerLaunchException.class)
public void testEmptyConfigurationOverride() {
try (CassandraContainer cassandraContainer = (CassandraContainer) new CassandraContainer()
Copy link
Member

Choose a reason for hiding this comment

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

Did you try to incorperate the proposed fix for omitting explicit cast mentioned by @bsideup?

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 did. Pushed commit several minutes ago. So no excess casts anymore 👍

@rnorth
Copy link
Member

rnorth commented Feb 11, 2018

We're shortly going to be merging #574, which changes our build system to Gradle. This is in part intended to make contributions of modules easier (per #564), but unfortunately means that for a short while your PR is going to show merge conflicts with the master branch.

I just want to let you know we don't want to create new work for you: we'll take care of the merge conflicts shortly. Please don't worry - we're grateful for your PR and want to help integrate it soon. Thank you.

description = "TestContainers :: Cassandra"

dependencies {
compile project(":database-commons"),
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add each dependency on its own line, with a compile? 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

* Can be used to obtain connections to Cassandra in the container
*/
public Cluster getCluster() {
return Cluster.builder()
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if returning Cluster.Builder is better or not, at least I expect some users to provide additional cluster configuration. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cluster.builder() is just static method (obliviously :) ). User can build Cluster instance himself with needed contact points, ports and other configuration.

getCluster() is just convenient method to get configured cluster instance. It can be useful when you have Cassandra only in tests (a bit synthetics case but still) and you don't have configure DataStax driver in you main configuration. Which you would do through system variables and methods getContainerIpAddress() and getMappedPort(...).

So there are two convenient ways to connect Cassandra in the container. No need to bring third with static builder.

}

private ResultSet performQuery(CassandraContainer cassandraContainer, String cql) {
Cluster explicitCluster = Cluster.builder()
Copy link
Member

Choose a reason for hiding this comment

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

cassandraContainer.getCluster() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's intentional. As you can see there are two performQuery methods. First builds cluster instance with general information from container (container ip and mapped port) and invokes second performQuery method which just uses passed cluster argument to perform query.
Second method is used directly in testCassandraGetCluster() test which creates cluster instance via getCluster() method and invokes performQuery() method.

Two ways of connection configuration are tested here.

@bduisenov
Copy link
Contributor

Hi there! Great PR!
any updates so far?

@AnkBurov
Copy link
Contributor Author

@bduisenov no activity from the code owners.

Use a fork if you need this feature.

@kiview
Copy link
Member

kiview commented Jun 8, 2018

@AnkBurov Sorry for not giving any feedback.
We'll look into this shortly.

@JakubKahovec
Copy link

Hi, any plans to merge it so it can be also used from testcontainers-scala ?

@platan
Copy link

platan commented Jul 4, 2018

@AnkBurov How can I use/test your fork? Can you provide a short instruction? Should I create artifacts on my own or there is some repo with with artifacts from your fork?

@AnkBurov
Copy link
Contributor Author

AnkBurov commented Jul 4, 2018

@platan hi. Just clone my fork and execute mvn install (optionally you can change version of the project to have distinct artifact names, for example fork.1.0). It will build and deploy artifacts to your local maven repository. Push them to your artifactory system and in your project add following dependencies (gradle format):
"org.testcontainers:testcontainers:fork.1.0"
"org.testcontainers:jdbc:fork.1.0"
"org.testcontainers:database-commons:fork.1.0"
"org.testcontainers:cassandra:fork.1.0"
and you will be able to use Cassandra container from this PR.

there is some repo with with artifacts from your fork?

I do have a repo with these artifacts but it's internal for a company I work for. Btw I can just give you jar files from there if you want.

@AnkBurov
Copy link
Contributor Author

AnkBurov commented Jul 4, 2018

"org.testcontainers:jdbc:fork.1.0" - is not needed for Cassandra container. It just contains useful feature from #575 PR - but it's only for JDBC so probably you won't need it.

@bsideup
Copy link
Member

bsideup commented Jul 10, 2018

@AnkBurov could you plz merge master into this PR to trigger the CircleCI?

@AnkBurov
Copy link
Contributor Author

@bsideup ok. I'll do it this evening.

@bsideup
Copy link
Member

bsideup commented Jul 10, 2018

@AnkBurov thank you!

@AnkBurov
Copy link
Contributor Author

@bsideup Oh, sorry, didn't check the schedule. Today is the day of Apache Ignite Moscow meetup. Will do it tomorrow.

@bsideup bsideup requested a review from rnorth as a code owner July 10, 2018 10:08
@bsideup
Copy link
Member

bsideup commented Jul 10, 2018

@AnkBurov I've triggered a dummy change :) We plan to cut a release today and would like to include your long awaited module :)

@bsideup bsideup added this to the next milestone Jul 10, 2018
@bsideup bsideup self-assigned this Jul 10, 2018
@AnkBurov
Copy link
Contributor Author

Caused by:
java.lang.IllegalStateException: Unsupported container type
at org.testcontainers.containers.wait.CassandraQueryWaitStrategy.getCassandraContainer(CassandraQueryWaitStrategy.java:54)

Looks like some code here in pr is quite obsolete comparing to current master and merge is needed.

@bsideup
Copy link
Member

bsideup commented Jul 10, 2018

@AnkBurov looks so. If you want, I can merge your PR to cassandra branch of the main repo and fix it myself :)

@AnkBurov
Copy link
Contributor Author

@bsideup personally I'm okay with this.
I will be able to fix it only tomorrow.

@bsideup
Copy link
Member

bsideup commented Jul 10, 2018

@AnkBurov ok, I'll do it later today. Sorry for taking so long, I hope we can finally deliver it 💪

@bsideup bsideup changed the base branch from master to cassandra July 10, 2018 10:38
@bsideup bsideup merged commit f06fd18 into testcontainers:cassandra Jul 10, 2018
@bsideup bsideup mentioned this pull request Jul 10, 2018
rnorth pushed a commit that referenced this pull request Apr 10, 2019
Bumps [amqp-client](https://github.com/rabbitmq/rabbitmq-java-client) from 5.6.0 to 5.7.0.
<details>
<summary>Release notes</summary>

*Sourced from [amqp-client's releases](https://github.com/rabbitmq/rabbitmq-java-client/releases).*

> ## 5.7.0
> # Changes between 5.6.0 and 5.7.0
> 
> This is a minor release with usability improvements and dependency upgrades. All users of the 5.x.x series are encouraged to upgrade to this version.
> 
> ## Improve logging for TLS connections
> 
> GitHub issue: [#441](https://github-redirect.dependabot.com/rabbitmq/rabbitmq-java-client/issues/441)
> 
> ## Don't panic when cancelling an unknown consumer
> 
> GitHub issue: [#525](https://github-redirect.dependabot.com/rabbitmq/rabbitmq-java-client/issues/525)
> 
> ## Bump dependencies
> 
> GitHub issue: [#607](https://github-redirect.dependabot.com/rabbitmq/rabbitmq-java-client/issues/607) 
> 
> ## 5.7.0.RC1
> # Changes between 5.6.0 and 5.7.0.RC1
> 
> This is a pre-release for 5.7.0, a maintenance release with usability improvements and dependency upgrades. All users of the 5.x.x series are encouraged to test this version.
> 
> ## Improve logging for TLS connections
> 
> GitHub issue: [#441](https://github-redirect.dependabot.com/rabbitmq/rabbitmq-java-client/issues/441)
> 
> ## Don't panic when cancelling an unknown consumer
> 
> GitHub issue: [#525](https://github-redirect.dependabot.com/rabbitmq/rabbitmq-java-client/issues/525)
> 
> ## Bump dependencies
> 
> GitHub issue: [#607](https://github-redirect.dependabot.com/rabbitmq/rabbitmq-java-client/issues/607) 
</details>
<details>
<summary>Changelog</summary>

*Sourced from [amqp-client's changelog](https://github.com/rabbitmq/rabbitmq-java-client/blob/v5.7.0/release-versions.txt).*

> RELEASE_VERSION="5.7.0"
> DEVELOPMENT_VERSION="5.8.0-SNAPSHOT"
</details>
<details>
<summary>Commits</summary>

- [`57aa724`](rabbitmq/rabbitmq-java-client@57aa724) [maven-release-plugin] prepare release v5.7.0
- [`1928ce8`](rabbitmq/rabbitmq-java-client@1928ce8) Set release version to 5.7.0
- [`9f2dffe`](rabbitmq/rabbitmq-java-client@9f2dffe) [maven-release-plugin] prepare for next development iteration
- [`e79ed4d`](rabbitmq/rabbitmq-java-client@e79ed4d) [maven-release-plugin] prepare release v5.7.0.RC1
- [`bb8f6ed`](rabbitmq/rabbitmq-java-client@bb8f6ed) Bump test dependencies
- [`953d255`](rabbitmq/rabbitmq-java-client@953d255) Bump dependencies
- [`e49f66d`](rabbitmq/rabbitmq-java-client@e49f66d) Add comment to explain decision tree
- [`365dd22`](rabbitmq/rabbitmq-java-client@365dd22) Add test for Channel#basicCancel with unknown consumer tag
- [`6081628`](rabbitmq/rabbitmq-java-client@6081628) Log warning when receiving basic.cancel for unknown consumer
- [`be57b26`](rabbitmq/rabbitmq-java-client@be57b26) Merge pull request [#548](https://github-redirect.dependabot.com/rabbitmq/rabbitmq-java-client/issues/548) from spring-operator/polish-urls-apache-license-5.x.x...
- Additional commits viewable in [compare view](rabbitmq/rabbitmq-java-client@v5.6.0...v5.7.0)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=com.rabbitmq:amqp-client&package-manager=gradle&previous-version=5.6.0&new-version=5.7.0)](https://dependabot.com/compatibility-score.html?dependency-name=com.rabbitmq:amqp-client&package-manager=gradle&previous-version=5.6.0&new-version=5.7.0)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
If all status checks pass Dependabot will automatically merge this pull request.

[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in the `.dependabot/config.yml` file in this repo:
- Update frequency (including time of day and day of week)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)

Finally, you can contact us by mentioning @dependabot.

</details>
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.

7 participants