Skip to content

Implement QuestDB Test container module#5995

Merged
eddumelendez merged 14 commits into
testcontainers:mainfrom
Vangreen:feautre/implement-questdb-module
Oct 21, 2022
Merged

Implement QuestDB Test container module#5995
eddumelendez merged 14 commits into
testcontainers:mainfrom
Vangreen:feautre/implement-questdb-module

Conversation

@Vangreen

Copy link
Copy Markdown
Contributor

Hi all, there is my implementation of QuestDB
https://github.com/questdb/questdb

@Vangreen Vangreen requested a review from a team October 18, 2022 16:44

@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 the PR @Vangreen ! I have added some suggestions. It would be great if some questions I left can be covered with real integration tests. Also, improve docs by mentioning the driver needed, you can see other database modules.

Comment thread docs/modules/databases/questdb.md Outdated
Comment thread modules/questdb/src/main/java/org/testcontainers/containers/QuestDBContainer.java Outdated
Comment thread modules/questdb/src/main/java/org/testcontainers/containers/QuestDBContainer.java Outdated
Comment thread modules/questdb/src/main/java/org/testcontainers/containers/QuestDBContainer.java Outdated
@kiview

kiview commented Oct 19, 2022

Copy link
Copy Markdown
Member

Hi @Vangreen, thanks for submitting the PR. However, it seems to be a duplicate of #5606.

@Vangreen

Copy link
Copy Markdown
Contributor Author

@eddumelendez Thanks for suggestions, I added them to pr.
@kiview I haven't notice it, I try to google it but nothing show up, I spoke with @jerrinot (author of #5606 ). I implement some of his suggestions in my PR.

@kiview

kiview commented Oct 19, 2022

Copy link
Copy Markdown
Member

@jerrinot Would you like this PR to supersede your original PR?

@jerrinot

Copy link
Copy Markdown
Contributor

@kiview indeed, I'm in touch with @Vangreen, and they adopted some ideas from my original PR.

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

@Vangreen thanks for addressing those comments so quickly! I left some suggestions.

Comment thread modules/questdb/src/main/java/org/testcontainers/containers/QuestDBContainer.java Outdated
Comment thread modules/questdb/src/main/java/org/testcontainers/containers/QuestDBContainer.java Outdated
Comment thread modules/questdb/build.gradle Outdated
Comment thread modules/questdb/src/main/java/org/testcontainers/containers/QuestDBContainer.java Outdated
Comment thread modules/questdb/src/main/java/org/testcontainers/containers/QuestDBContainer.java Outdated
@Vangreen

Copy link
Copy Markdown
Contributor Author

@eddumelendez I have add your suggestion to PR. Tkanks

Comment thread modules/questdb/src/main/java/org/testcontainers/containers/QuestDBContainer.java Outdated
Comment thread modules/questdb/src/main/java/org/testcontainers/containers/QuestDBContainer.java Outdated

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

LGTM, thanks for working in the suggestions 👍

@kiview kiview requested a review from eddumelendez October 21, 2022 09:00
@eddumelendez eddumelendez merged commit 643b815 into testcontainers:main Oct 21, 2022
@eddumelendez

Copy link
Copy Markdown
Member

thank you so much @Vangreen ! the module now is part of Testcontainers in main branch. 🎉

@eddumelendez eddumelendez added this to the next milestone Oct 21, 2022
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.

4 participants