Skip to content

Add CrateDB testcontainers#6790

Merged
eddumelendez merged 11 commits into
testcontainers:mainfrom
matriv:add-cratedb
Mar 31, 2023
Merged

Add CrateDB testcontainers#6790
eddumelendez merged 11 commits into
testcontainers:mainfrom
matriv:add-cratedb

Conversation

@matriv

@matriv matriv commented Mar 20, 2023

Copy link
Copy Markdown
Contributor

Addresses: #6789

@matriv matriv requested a review from a team 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

@eddumelendez eddumelendez left a comment

Copy link
Copy Markdown
Member

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.

Comment thread docs/modules/databases/cratedb.md Outdated
Comment thread modules/cratedb/build.gradle Outdated
Comment thread modules/cratedb/src/main/java/org/testcontainers/containers/CrateDBContainer.java Outdated
Comment thread modules/cratedb/src/test/java/org/testcontainers/CrateDBTestImages.java
Comment thread modules/jdbc/src/main/java/org/testcontainers/jdbc/ConnectionUrl.java Outdated
Comment thread modules/jdbc/src/test/java/org/testcontainers/jdbc/ConnectionUrlTest.java Outdated
Comment thread modules/jdbc/src/test/java/org/testcontainers/jdbc/ConnectionUrlTest.java Outdated
@matriv

matriv commented Mar 29, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

eddumelendez commented Mar 30, 2023

Copy link
Copy Markdown
Member

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

@matriv

matriv commented Mar 30, 2023

Copy link
Copy Markdown
Contributor Author

@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

matriv commented Mar 30, 2023

Copy link
Copy Markdown
Contributor Author

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

eddumelendez commented Mar 31, 2023

Copy link
Copy Markdown
Member

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

@matriv

matriv commented Apr 1, 2023

Copy link
Copy Markdown
Contributor Author

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