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

Implement QuestDB Test container module #5995

Merged
merged 14 commits into from Oct 21, 2022
Merged

Implement QuestDB Test container module #5995

merged 14 commits into from Oct 21, 2022

Conversation

Vangreen
Copy link
Contributor

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

@Vangreen Vangreen requested a review from a team as a code owner October 18, 2022 16:44
eddumelendez
eddumelendez requested changes Oct 18, 2022
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 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.

docs/modules/databases/questdb.md Outdated Show resolved Hide resolved
modules/questdb/src/main/java/org/testcontainers/containers/QuestDBContainer.java Outdated Show resolved Hide resolved
modules/questdb/src/main/java/org/testcontainers/containers/QuestDBContainer.java Outdated Show resolved Hide resolved
modules/questdb/src/main/java/org/testcontainers/containers/QuestDBContainer.java Outdated Show resolved Hide resolved
@kiview
Copy link
Member

kiview commented Oct 19, 2022

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

@Vangreen
Copy link
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
Copy link
Member

kiview commented Oct 19, 2022

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

@jerrinot
Copy link
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
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.

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

modules/questdb/src/main/java/org/testcontainers/containers/QuestDBContainer.java Outdated Show resolved Hide resolved
modules/questdb/src/test/java/org/testcontainers/junit/questdb/SimpleQuestDBTest.java Outdated Show resolved Hide resolved
modules/questdb/src/main/java/org/testcontainers/containers/QuestDBContainer.java Show resolved Hide resolved
modules/questdb/src/main/java/org/testcontainers/containers/QuestDBContainer.java Outdated Show resolved Hide resolved
modules/questdb/build.gradle Outdated Show resolved Hide resolved
modules/questdb/src/main/java/org/testcontainers/containers/QuestDBContainer.java Outdated Show resolved Hide resolved
modules/questdb/src/main/java/org/testcontainers/containers/QuestDBContainer.java Outdated Show resolved Hide resolved
@Vangreen
Copy link
Contributor Author

@eddumelendez I have add your suggestion to PR. Tkanks

kiview
kiview approved these changes Oct 21, 2022
View reviewed changes
Copy link
Member

@kiview kiview left a comment

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 👍

@eddumelendez eddumelendez merged commit 643b815 into testcontainers:main Oct 21, 2022
@eddumelendez
Copy link
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

@jerrinot jerrinot jerrinot left review comments

@eddumelendez eddumelendez eddumelendez approved these changes

@kiview kiview kiview approved these changes

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.

None yet
4 participants
@Vangreen @kiview @jerrinot @eddumelendez