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.

Sign up for GitHub

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

Jump to bottom

Add CrateDB testcontainers #6790

Merged
merged 11 commits into from
Mar 31, 2023
Merged

Add CrateDB testcontainers #6790

merged 11 commits into from
Mar 31, 2023

Conversation

matriv
Copy link
Contributor

@matriv matriv commented Mar 20, 2023

Addresses: #6789

@matriv matriv requested a review from a team as a code owner March 20, 2023 14:21
@matriv matriv force-pushed the add-cratedb branch 3 times, most recently from b4ba30e to ea781eb Compare March 20, 2023 19:07
@eddumelendez eddumelendez changed the title Add CrateDB testcontainer Add CrateDB testcontainers Mar 28, 2023
eddumelendez
eddumelendez requested changes Mar 29, 2023
View reviewed 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.

Thanks for your contribution, @matriv ! I have left some comments and we suggest to remove the legacy driver given it is a new module.

Also, please do not use force push.

docs/modules/databases/cratedb.md Outdated Show resolved Hide resolved
modules/cratedb/build.gradle Outdated Show resolved Hide resolved
modules/cratedb/src/main/java/org/testcontainers/containers/CrateDBContainer.java Outdated Show resolved Hide resolved
modules/cratedb/src/test/java/org/testcontainers/CrateDBTestImages.java Show resolved Hide resolved
modules/jdbc/src/main/java/org/testcontainers/jdbc/ConnectionUrl.java Outdated Show resolved Hide resolved
modules/jdbc/src/test/java/org/testcontainers/jdbc/ConnectionUrlTest.java Outdated Show resolved Hide resolved
modules/jdbc/src/test/java/org/testcontainers/jdbc/ConnectionUrlTest.java Outdated Show resolved Hide resolved
@matriv
Copy link
Contributor Author

matriv commented Mar 29, 2023 •
edited
Loading

Thx a lot for reviewing the PR @eddumelendez !

we suggest to remove the legacy driver given it is a new module.

There are certain places that still require the legacy driver, because if vanilla Postgres is used,
they assume things about the internal structures/tables where CrateDB is not 100% combatible, see this example

Although, there is a workaround for such cases, so if you prefer not to include the legacy version, I'd be happy to remove it.

Also, please do not use force push.

Apologies, I didn't change anything in my PR, just rebased with main. I won't force push again of course, now that the PR is in review.

@eddumelendez
Copy link
Member

@matriv thanks for the quick update! Please, let's remove the legacy version

@matriv
Copy link
Contributor Author

matriv commented Mar 30, 2023

@matriv thanks for the quick update! Please, let's remove the legacy version

It's reasonable, thx! please also check: #6790 (comment)

@matriv matriv requested a review from eddumelendez March 30, 2023 08:48
@matriv
Copy link
Contributor Author

matriv commented Mar 30, 2023

I believe I've had addressed your comments so far, removed the legacy version, please have another look, many thx!

@eddumelendez eddumelendez added this to the next milestone Mar 31, 2023
@eddumelendez eddumelendez merged commit b221f3f into testcontainers:main Mar 31, 2023
@eddumelendez
Copy link
Member

Thanks for your contribution, @matriv ! This is now merged in main branch and it will be part of the next release.

@matriv
Copy link
Contributor Author

matriv commented Apr 1, 2023

Thx a lot for reviewing and merging!

@matriv matriv deleted the add-cratedb branch April 1, 2023 05:57
This was referenced Apr 5, 2023
Open
Merged
@amotl amotl mentioned this pull request Oct 10, 2023
Open
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddumelendez eddumelendez eddumelendez approved these changes

Assignees
No one assigned
Labels
type/docs type/new module
Projects
None yet
Milestone
1.18.0
Development

Successfully merging this pull request may close these issues.
2 participants
@matriv @eddumelendez