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

DevServices Support #14960

Merged
merged 2 commits into from
Feb 24, 2021
Merged

DevServices Support #14960

merged 2 commits into from
Feb 24, 2021

Conversation

stuartwdouglas
Copy link
Member

@stuartwdouglas stuartwdouglas commented Feb 10, 2021 •
edited
Loading

DevServices allows for zero configuration databases in dev
and test mode, either starting the databases using
testcontainers, or creating them in process.

@ghost ghost added area/core area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/hibernate-orm Hibernate ORM area/hibernate-reactive Hibernate Reactive area/persistence OBSOLETE, DO NOT USE area/reactive area/testing labels Feb 10, 2021
@cmoulliard
Copy link

testcontainers

What is a testcontainer: a pod, a docker/podman process, ... ?

@geoand
Copy link
Contributor

geoand commented Feb 10, 2021

Awesome stuff! I'll try and play around with it tomorrow

@stuartwdouglas
Copy link
Member Author

https://www.testcontainers.org/

I could do it directly using docker but would likely end up just reimplementing a lot of what is there

famod
famod reviewed Feb 10, 2021
View reviewed changes
build-parent/pom.xml
@@ -481,14 +481,12 @@
<exclude>io.rest-assured:*</exclude>
<exclude>org.assertj:*</exclude>
<exclude>org.junit.jupiter:*</exclude>
<exclude>org.testcontainers:*</exclude>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose you removed it because it pops up as a transitive dependency in many places now?
I wonder whether we can keep it by making the rule non-transitive? This would still cover the main problem that #13919 tried to address.

@stuartwdouglas stuartwdouglas marked this pull request as ready for review February 10, 2021 23:04
@knutwannheden
Copy link
Contributor

This looks really nice and we are really looking forward to this (and we can stop developing our own extension which we created with the same purpose).

Question: AFAICT MSSQL is the only built-in database kind which doesn't have DevDB support. Will that still be includee in this PR?

@stuartwdouglas
Copy link
Member Author

I am not sure about the licensing for MSSQL and DB2, technically it is easy to do, but I need to investigate how to handle the legalities around accepting the EULA.

@knutwannheden
Copy link
Contributor

I see. As you have probably seen Testcontainers requires placing a container-license-acceptance.txt file on the classpath containing the Docker image name.

I suppose it should be easy for us to supply our own DevDB support for MSSQL (for our own Quarkus applications) in case you can't work the legalities out just yet.

@stuartwdouglas
Copy link
Member Author

That approach is probably fine. I will check, but either way I will hold off until the initial PR is merged.

@stuartwdouglas stuartwdouglas force-pushed the testcontainers-demo branch 2 times, most recently from 226761b to c1203d1 Compare February 11, 2021 05:40
@ghost ghost added the area/panache label Feb 11, 2021
geoand
geoand reviewed Feb 11, 2021
View reviewed changes
Copy link
Contributor

@geoand geoand left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fantastic work!

As it mostly touches datasource related stuff, I think we need a review from @Sanne and / or @gsmet

@geoand geoand mentioned this pull request Feb 11, 2021
Closed
3 tasks
@stuartwdouglas stuartwdouglas force-pushed the testcontainers-demo branch 4 times, most recently from 3416f08 to 366a9b4 Compare February 18, 2021 08:41
@stuartwdouglas
Copy link
Member Author

Hopefully the log CI issues are fixed now. At the moment the container logs display before the banner, but I can fix that if needed.

@geoand geoand mentioned this pull request Feb 19, 2021
Closed
gsmet
gsmet requested changes Feb 19, 2021
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the doc a bit and suggested a few improvements.

docs/src/main/asciidoc/datasource.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/datasource.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/datasource.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/datasource.adoc
@@ -382,6 +403,10 @@ quarkus.datasource.inventory.jdbc.max-size=12
Notice there is an extra bit in the key.
The syntax is as follows: `quarkus.datasource.[optional name.][datasource property]`.

NOTE: Even when only one database extension is installed named databases need to specify at least one build time
property so that Quarkus knows they exist. Generally this will be the `db-kind` property, although you can also
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, typically, that's one of the reasons why I didn't like having db-kind being optional. I'm still not convinced it's a good idea. I suppose we will see...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why I think it is ok is that the single DB + single driver is the 99% use case, and we can effectively make this configuration free.

docs/src/main/asciidoc/datasource.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/datasource.adoc Outdated Show resolved Hide resolved
@stuartwdouglas
Copy link
Member Author

Doc fixes applied.

@geoand geoand requested a review from gsmet February 23, 2021 07:17
@stuartwdouglas
Copy link
Member Author

What else is required to get this in?

gastaldi
gastaldi reviewed Feb 23, 2021
View reviewed changes
bom/application/pom.xml Outdated Show resolved Hide resolved
@geoand
Copy link
Contributor

geoand commented Feb 24, 2021

This is the highlight of the next release and the more baking time we have the better.
I propose we get it ASAP if there are no objections

@stuartwdouglas @gsmet
DevServices Support
f4cba6a 
DevServices allows for zero configuration databases in dev
and test mode, either starting the databases using
testcontainers, or creating them in process.
@stuartwdouglas @gsmet
Display DevServices logs at build time
88172b7
@gsmet
Copy link
Member

gsmet commented Feb 24, 2021

I applied the latest suggestions, rebased and squashed. Let's wait for CI and merge.

@gsmet gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 24, 2021
@geoand geoand merged commit 5116e62 into quarkusio:master Feb 24, 2021
@quarkus-bot quarkus-bot bot added this to the 1.13 - master milestone Feb 24, 2021
@famod famod removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 24, 2021
@maxandersen maxandersen added the triage/qe? Issue could use a quality focused review to further harden it label Feb 24, 2021
@geoand geoand mentioned this pull request Feb 26, 2021
Closed
@knutwannheden knutwannheden mentioned this pull request Mar 8, 2021
Closed
@jsmrcka jsmrcka mentioned this pull request Mar 31, 2021
Merged
@radcortez radcortez mentioned this pull request Apr 1, 2021
Closed
@kshpak kshpak mentioned this pull request Apr 22, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@famod famod famod left review comments

@machi1990 machi1990 machi1990 left review comments

@geoand geoand geoand left review comments

@gastaldi gastaldi gastaldi left review comments

@maxandersen maxandersen maxandersen left review comments

@gsmet gsmet gsmet approved these changes

@Sanne Sanne Awaiting requested review from Sanne

Assignees
No one assigned
Labels
area/core area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/flyway area/hibernate-orm Hibernate ORM area/hibernate-reactive Hibernate Reactive area/liquibase area/logging area/panache area/persistence OBSOLETE, DO NOT USE area/reactive area/testing release/noteworthy-feature triage/qe? Issue could use a quality focused review to further harden it
Projects
None yet
Milestone
1.13.0.CR1
Development

Successfully merging this pull request may close these issues.
10 participants
@stuartwdouglas @cmoulliard @geoand @knutwannheden @maxandersen @gastaldi @Sanne @gsmet @machi1990 @famod