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

Add YugabyteDB module #4372

Merged
merged 18 commits into from Nov 15, 2022
Merged

Conversation

srinivasa-vasu
Copy link
Contributor

@srinivasa-vasu srinivasa-vasu commented Aug 15, 2021

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

  • Should be a Docker Hub official image, or published by a reputable source (ideally the company or organisation that officially supports the technology)
  • Should have a verifiable open source Dockerfile and a way to view the history of changes
  • MUST show general good practices regarding container image tagging - e.g. we do not use latest tags, and we do not use tags that may be mutated in the future
  • MUST be legal for Testcontainers developers and Testcontainers users to pull and use. Mechanisms exist to allow EULA acceptance to be signalled, but images that can be used without a licence are greatly preferred.

Module dependencies

  • The module should use as few dependencies as possible,
  • Regarding libraries, either:
  • they should be compileOnly if they are likely to already be on the classpath for users' tests (e.g. client libraries or drivers)
  • they can be implementation (and thus transitive dependencies) if they are very unlikely to conflict with users' dependencies.
  • If client libraries are used to test or use the module, these MUST be legal for Testcontainers developers and Testcontainers users to download and use.

API (e.g. MyModuleContainer class)

  • Favour doing the right thing, and least surprising thing, by default
  • Ensure that method and parameter names are easy to understand. Many users will ignore documentation, so IDE-based substitutes (autocompletion and Javadocs) should be intuitive.
  • The module's public API should only handle standard JDK data types and MUST not expose data types that come from compileOnly dependencies. This is to reduce the risk of compatibility problems with future versions of third party libraries.

Documentation
Every module MUST have a dedicated documentation page containing:

  • A high level overview
  • A usage example
  • If appropriate, basic API documentation or further usage guidelines
  • Dependency information
  • Acknowledgements, if appropriate
  • Consider that many users will not read the documentation pages - even if the first person to add it to a project does, people reading/updating the code in the future may not. Try and avoid the need for critical knowledge that is only present in documentation.

@nchandrappa
Copy link

@bsideup @kiview @rnorth Hi All, There is a lot of interest in our community to get YugabyteDB support in TestContainers project, please let us know what are steps required for getting the following PR merged and bringing support for YugabyteDB in Testcontianers project. Thanks, Nikhil

@paskos
Copy link

paskos commented Dec 14, 2021

Hi,
We're about to start using yugabyte in our project. Is there an ETA on this PR ?

@srinivasa-vasu
Copy link
Contributor Author

@bsideup @kiview @rnorth - Pls let us know if you need anything from our side to get this PR reviewed and merged. There is a lot of interest in the YB community to use this. Thnx.

Copy link
Member

@eddumelendez eddumelendez left a 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)?

docs/modules/databases/yugabytedb.md Outdated Show resolved Hide resolved
docs/modules/databases/yugabytedb.md Outdated Show resolved Hide resolved
@srinivasa-vasu
Copy link
Contributor Author

Thanks for the PR @srinivasa-vasu ! LGTM overall but WDYT about having YugabyteContainer only and provide something like withAPI(YugabyteAPI.YSQL), withAPI(YugabyteAPI.YCQL)?

@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.

@eddumelendez
Copy link
Member

thanks for addressing the comments so fast! We will take a look at the changes

Copy link
Member

@eddumelendez eddumelendez left a 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.

Comment on lines 116 to 126
@Override
protected void containerIsStarted(InspectContainerResponse containerInfo) {
if (initScript != null) {
ScriptUtils.runInitScript(new YugabyteDBYCQLDelegate(getSessionBuilder()), initScript);
}
}
Copy link
Member

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 ?

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'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?

@eddumelendez
Copy link
Member

eddumelendez commented Aug 4, 2022

@srinivasa-vasu can you please add yugabyte to

  • .github/ISSUE_TEMPLATE/bug_report.yaml
  • .github/ISSUE_TEMPLATE/enhancement.yaml
  • .github/ISSUE_TEMPLATE/feature.yaml
  • .github/dependabot.yml
  • .github/labeler.yml

@eddumelendez eddumelendez changed the title yugabytedb :: Testcontainer implementation for YSQL and YCQL APIs :: initial-commit Add YugabyteDB module Aug 6, 2022
@srinivasa-vasu
Copy link
Contributor Author

@eddumelendez - anything pending from my side?

Copy link
Member

@eddumelendez eddumelendez left a 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.

Comment on lines 78 to 85
/**
* @param initScript path of the initialization script file
* @return {@link YugabyteDBYCQLContainer} instance
*/
public YugabyteDBYCQLContainer withInitScript(String initScript) {
this.initScript = initScript;
return this;
}
Copy link
Member

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?

Copy link
Member

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.

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'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?

Copy link
Member

@eddumelendez eddumelendez left a 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.

*/
@RequiredArgsConstructor
@Slf4j
public final class YugabyteDBYCQLWaitStrategy 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.

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?

eddumelendez
eddumelendez previously approved these changes Nov 8, 2022
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>
@eddumelendez eddumelendez added this to the next milestone Nov 15, 2022
@eddumelendez eddumelendez merged commit 858dd0a into testcontainers:main Nov 15, 2022
@eddumelendez
Copy link
Member

thank you so much for your contribution @srinivasa-vasu ! This is now in main branch and it will be part of the next release.

@srinivasa-vasu
Copy link
Contributor Author

Thanks @eddumelendez

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.

None yet

5 participants