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

Make TimescaleDB available with JDBC syntax #3891

Merged
merged 17 commits into from
Sep 23, 2021

Conversation

raynigon
Copy link
Contributor

@raynigon raynigon commented Mar 17, 2021

Fix #3890

The Syntax for the use of a TimescaleDB Testcontainer should be jdbc:tc:timescaledb:///databasename or jdbc:tc:timescaledb:2.1.0-pg13:///databasename if a Tag is given. This should allow the user to use the container without any further configuration.

Checklist

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.

The Syntax for the use of a TimescaleDB Testcontainer should be jdbc:tc:timescaledb:///databasename or jdbc:tc:timescaledb:2.1.0-pg13:///databasename if a Tag is given. This should allow the user to use the container without any further configuration.
The DockerImage has to be compatible with PostgreSQL
The DockerImage has to be compatible with PostgreSQL
Comment on lines +120 to 124
if (databaseType.equalsIgnoreCase("postgresql") ||
databaseType.equalsIgnoreCase("postgis") ||
databaseType.equalsIgnoreCase("timescaledb")) {
databaseQuery = "SELECT CURRENT_DATABASE()";
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is pretty ugly and should be made more generic, but that would be a different PR.

Copy link
Member

@rnorth rnorth left a comment

Choose a reason for hiding this comment

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

I think I'm happy with this - it follows the pattern used for PostGIS already (although has more by way of tests, which is good).

One general topic, which I think we should consider outside of this PR, is recommendations for usage of PostGIS/TimescaleDB outside of a JDBC URL usage mode. I know it's as simple as creating a PostgreSQLContainer with a different image, but this isn't particularly discoverable or documented.

Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your contribution @raynigon.

@rnorth
I was thinking the same thing with regards to the similar Postgis scenario as well.

@kiview kiview added this to the next milestone Sep 23, 2021
@kiview kiview merged commit f178e91 into testcontainers:master Sep 23, 2021
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.

Make TimescaleDB available with JDBC syntax
3 participants