-
-
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
Add YugabyteDB module #4372
Add YugabyteDB module #4372
Conversation
a4b6e20
to
45273e7
Compare
Hi, |
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.
Thanks for the PR @srinivasa-vasu ! LGTM overall but WDYT about having YugabyteContainer
only and provide something like withAPI(YugabyteAPI.YSQL)
, withAPI(YugabyteAPI.YCQL)
?
modules/yugabytedb/src/main/java/org/testcontainers/containers/YugabyteContainerConstants.java
Outdated
Show resolved
Hide resolved
@eddumelendez - Thanks for reviewing this. The APIs' are different, one is fully relational, and the other is semi and Cassandra compatible. Will end up with a lot of conditional blocks. Having it separate may be a cleaner abstraction. |
thanks for addressing the comments so fast! We will take a look at the changes |
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.
leaving additional comments and still trying to figure it out how to handle one container class for both APIs.
...les/yugabytedb/src/test/java/org/testcontainers/junit/yugabytedb/YugabyteDBYSQLUnitTest.java
Outdated
Show resolved
Hide resolved
...les/yugabytedb/src/test/java/org/testcontainers/junit/yugabytedb/YugabyteDBYCQLUnitTest.java
Outdated
Show resolved
Hide resolved
modules/yugabytedb/src/main/java/org/testcontainers/containers/YugabyteDBYCQLContainer.java
Outdated
Show resolved
Hide resolved
@Override | ||
protected void containerIsStarted(InspectContainerResponse containerInfo) { | ||
if (initScript != null) { | ||
ScriptUtils.runInitScript(new YugabyteDBYCQLDelegate(getSessionBuilder()), initScript); | ||
} | ||
} |
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.
initScript
is a very convenient method but there are existing tools such as liquibase-cassandra with this purpose and execInContainer()
would help to perform similar operations. Doing so, we can remove YugabyteDBYCQLDelegate
. WDYT @kiview ?
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've decoupled the Cassandra client library dependency but kept the API as is. Removed Cassandra dependency from YugabyteDBYCQLDelegate
. This API may be useful for standalone services. Let me know WDYT?
ab4aa7c
to
47a6e03
Compare
modules/yugabytedb/src/main/java/org/testcontainers/containers/YugabyteDBYCQLContainer.java
Outdated
Show resolved
Hide resolved
modules/yugabytedb/src/main/java/org/testcontainers/containers/YugabyteDBYSQLContainer.java
Outdated
Show resolved
Hide resolved
@srinivasa-vasu can you please add yugabyte to
|
a91af2d
to
bbaa4aa
Compare
@eddumelendez - anything pending from my side? |
435c987
to
c7d68ea
Compare
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.
@srinivasa-vasu sorry for the delay. I have left a couple of comments about using assertj instead.
modules/yugabytedb/src/test/java/org/testcontainers/junit/yugabytedb/YugabyteDBYCQLTest.java
Outdated
Show resolved
Hide resolved
modules/yugabytedb/src/test/java/org/testcontainers/junit/yugabytedb/YugabyteDBYSQLTest.java
Outdated
Show resolved
Hide resolved
/** | ||
* @param initScript path of the initialization script file | ||
* @return {@link YugabyteDBYCQLContainer} instance | ||
*/ | ||
public YugabyteDBYCQLContainer withInitScript(String initScript) { | ||
this.initScript = initScript; | ||
return this; | ||
} |
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 should not be supported because Testcontainers responsibility is about taking care of the service initialization. There are other tools which can help about keyspace and data management such as liquibase-cassandra. Also, that way we are avoiding being tight to a Cassandra library version. @kiview WDYT?
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 agree with @eddumelendez. Adding such an API increases complexity more than necessary.
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've decoupled the Cassandra client library dependency but kept the API as is. This API may be useful for standalone services. Let me know WDYT?
c7d68ea
to
dba2a50
Compare
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.
Just some suggestions in order to pass checks. Make sure to perform ./gradlew checkstyleMain checkstyleTest spotlessApply
before pushing changes to make sure all checks are ok.
...ugabytedb/src/test/java/org/testcontainers/jdbc/yugabytedb/YugabyteDBYSQLJDBCDriverTest.java
Outdated
Show resolved
Hide resolved
...ugabytedb/src/test/java/org/testcontainers/jdbc/yugabytedb/YugabyteDBYSQLJDBCDriverTest.java
Outdated
Show resolved
Hide resolved
...ugabytedb/src/test/java/org/testcontainers/jdbc/yugabytedb/YugabyteDBYSQLJDBCDriverTest.java
Outdated
Show resolved
Hide resolved
*/ | ||
@RequiredArgsConstructor | ||
@Slf4j | ||
public final class YugabyteDBYCQLWaitStrategy extends AbstractWaitStrategy { |
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 indeed now looks very convenient. I wonder if can do something similar to our existing CassandraContainer
and rid of the cassandra driver dependency at all, at least for now.
@srinivasa-vasu to give you a little more context, on sql there are some issues due to how each database handles the scripts. We recommend using tools such as liquibase or flyway in order to execute scripts that are specialized for that kind of jobs. TBH, not sure if we will have similar issues with CQL and for that reason we will try to avoid it. IMO, we should avoid it and promote best practices around database management in general. No need to take action yet on this. @kiview WDYT?
dba2a50
to
0af5d61
Compare
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
…acking Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
0af5d61
to
1bc628e
Compare
thank you so much for your contribution @srinivasa-vasu ! This is now in |
Thanks @eddumelendez |
Every item on this list will require judgement by the Testcontainers core maintainers. Exceptions will sometimes be possible; items with should are more likely to be negotiable than those items with must.
Default docker image
Module dependencies
API (e.g. MyModuleContainer class)
Documentation
Every module MUST have a dedicated documentation page containing: