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

@Vangreen Vangreen commented Oct 18, 2022

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 requested changes Oct 18, 2022
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 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

Vangreen commented Oct 19, 2022

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

jerrinot commented Oct 19, 2022

Copy link
Copy Markdown
Contributor

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

@jerrinot jerrinot mentioned this pull request Oct 19, 2022
Closed
eddumelendez
eddumelendez requested changes Oct 19, 2022
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.

@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/test/java/org/testcontainers/junit/questdb/SimpleQuestDBTest.java Outdated
Comment thread modules/questdb/src/main/java/org/testcontainers/containers/QuestDBContainer.java
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

Vangreen commented Oct 20, 2022

Copy link
Copy Markdown
Contributor Author

@eddumelendez I have add your suggestion to PR. Tkanks

kiview
kiview reviewed Oct 20, 2022
View reviewed changes
Comment thread modules/questdb/src/main/java/org/testcontainers/containers/QuestDBContainer.java Outdated
kiview
kiview reviewed Oct 20, 2022
View reviewed changes
Comment thread modules/questdb/src/main/java/org/testcontainers/containers/QuestDBContainer.java Outdated
kiview
kiview requested changes Oct 20, 2022
View reviewed changes
Comment thread modules/questdb/src/main/java/org/testcontainers/containers/QuestDBContainer.java
kiview
kiview approved these changes Oct 21, 2022
View reviewed changes

@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

eddumelendez commented Oct 21, 2022

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

Reviewers

@eddumelendez eddumelendez eddumelendez approved these changes

@kiview kiview kiview approved these changes

+1 more reviewer

@jerrinot jerrinot jerrinot left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

type/docs type/new module

Projects

None yet

Milestone

1.17.6

Development

Successfully merging this pull request may close these issues.

4 participants

@Vangreen @kiview @jerrinot @eddumelendez