-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
Add Presto container #2196
Add Presto container #2196
Conversation
This comment has been minimized.
This comment has been minimized.
@findepi I just triggered a rerun |
a85547b
to
d14243f
Compare
This comment has been minimized.
This comment has been minimized.
Hi @findepi, thanks for taking time to start a contribution. Please could you run through this section of the docs around new module contributions? https://www.testcontainers.org/contributing/#contributing-new-modules It would be helpful if you could complete the checklist so that we can review efficiently! Thank you |
2e08dda
to
5e46a43
Compare
@rnorth sure I also added a docs page, following PostgreSQL and MariaDB examples. Let me know what is the next step. |
4bc6b36
to
08fa3e7
Compare
+1 looking forward to this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry this has taken a long time to get to. I'm happy with this, and I think we should proceed. I hope you don't mind but I pushed a couple of trivial docs-related tweaks to the PR branch already, and just have one final comment. I hope this is OK!
Thanks again for the contribution and all your hard work.
@@ -0,0 +1,138 @@ | |||
/* | |||
* Licensed under the Apache License, Version 2.0 (the "License"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testcontainers is MIT licenced, and it looks like this is the only file with an Apache header. Is that an error?
I'd prefer it if we could keep all source files similarly licenced, just to avoid confusion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out & apologies.
@@ -53,6 +53,7 @@ nav: | |||
- modules/databases/neo4j.md | |||
- modules/databases/oraclexe.md | |||
- modules/databases/postgres.md | |||
- modules/databases/presto.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I squash this with the first commit
@@ -1,5 +1,8 @@ | |||
# Presto Module | |||
|
|||
!!! note |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -0,0 +1,138 @@ | |||
/* | |||
* Licensed under the Apache License, Version 2.0 (the "License"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out & apologies.
@rnorth thanks for the review! updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@findepi perfect! Let's merge :D
* Add Presto container * Add incubating note to module docs Co-authored-by: Richard North <[email protected]>
Released in 1.13.0! Thanks for the contribution @findepi 😃 |
* Add Presto container * Add incubating note to module docs Co-authored-by: Richard North <[email protected]>
Default docker image
Module dependencies
API (e.g. MyModuleContainer class)
Documentation