-
-
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 TiDB module #5511
Add TiDB module #5511
Conversation
PTAL @eddumelendez @kiview |
One month passed, PTAL. |
modules/tidb/src/main/java/org/testcontainers/containers/TiDBContainer.java
Outdated
Show resolved
Hide resolved
modules/tidb/src/main/java/org/testcontainers/containers/TiDBContainer.java
Outdated
Show resolved
Hide resolved
Hey @Icemap, thank you for the contribution, we did not yet have time to review it, sorry. |
Please run |
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 @Icemap ! I've left some comments
...main/resources/META-INF/services/org.testcontainers.containers.JdbcDatabaseContainerProvider
Outdated
Show resolved
Hide resolved
modules/tidb/src/main/java/org/testcontainers/containers/TiDBContainer.java
Outdated
Show resolved
Hide resolved
super(dockerImageName); | ||
dockerImageName.assertCompatibleWith(DEFAULT_IMAGE_NAME); | ||
|
||
addExposedPorts(TIDB_PORT, REST_API_PORT); |
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.
both ports should be exposed? or only 4000
?
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.
Both ports should be exposed. TIDB_PORT
is 4000, and REST_API_PORT
is 10080.
modules/tidb/src/test/java/org/testcontainers/junit/tidb/SimpleTiDBTest.java
Outdated
Show resolved
Hide resolved
@kiview Thanks for the review. And I got this error diffplug/spotless#834 at Java 17 when I run
|
...main/resources/META-INF/services/org.testcontainers.containers.JdbcDatabaseContainerProvider
Outdated
Show resolved
Hide resolved
@eddumelendez does this PR need to change [maintainers note] can we automate the check for new modules somehow? |
modules/tidb/src/main/java/org/testcontainers/containers/TiDBContainer.java
Outdated
Show resolved
Hide resolved
modules/tidb/src/main/java/org/testcontainers/containers/TiDBContainer.java
Outdated
Show resolved
Hide resolved
…ontainer.java Co-authored-by: Sergei Egorov <bsideup@gmail.com>
…ontainer.java Co-authored-by: Sergei Egorov <bsideup@gmail.com>
modules/tidb/src/test/java/org/testcontainers/junit/tidb/SimpleTiDBTest.java
Outdated
Show resolved
Hide resolved
@bsideup Yes, we need to touch those with new modules. We can take care of this for now. |
modules/tidb/src/main/java/org/testcontainers/containers/TiDBContainer.java
Outdated
Show resolved
Hide resolved
modules/tidb/src/main/java/org/testcontainers/containers/TiDBContainer.java
Outdated
Show resolved
Hide resolved
Thanks a lot for all your work @Icemap. I saw you recently started to do some more commits. Are we good to go from your side or is there something you want to work on a bit more? |
|
Recently commits were to fix CI errors. @kiview |
Merged, thanks for the great contribution and collaboration @Icemap 🙌 |
Add TiDB test container support.