Unify devservices datatabase, username and password and allow configuration #25288

newur · 2022-05-01T14:03:21Z

I wanted to inspect the Postgres database started by devservices. While searching for the login credentials I noticed that different databases use different credentials and db names. I think it is nicer to use the same for all. Bonus, at least for Postgres the login via psql becomes shorter when the username and db name are equal

psql -h localhost -p <randomPort> -U quarkus

instead of

psql -h localhost -p <randomPort> -U quarkus -d default

I did not find the information how to connect to the devservices datasources in the docu. Would be worth to add it IMO. Any hints where I should put it?

gsmet

I think that's indeed a very good idea, thanks.

Only one small gripe: we avoid star imports in Quarkus. Any chance you could remove them?

gsmet · 2022-05-02T08:44:55Z

Also please squash all the commits when done.

gsmet · 2022-05-03T11:55:53Z

I force pushed a formatting fix to your branch.

newur · 2022-05-04T21:43:04Z

DB2 really putted on a fight. Will the light prevail now or is it another round? ¯_(ツ)_/¯

gsmet

Please see my comments. There is a big mix up between username and password in the patch.

...yment-spi/src/main/java/io/quarkus/datasource/deployment/spi/DatabaseDefaultSetupConfig.java

...adb/src/main/java/io/quarkus/devservices/mariadb/deployment/MariaDBDevServicesProcessor.java

gsmet · 2022-05-05T10:10:17Z

...test/java/io/quarkus/jdbc/postgresql/deployment/DevServicesPostgresqlDatasourceTestCase.java

@@ -48,7 +48,7 @@ public void testDatasource() throws Exception {
            fail("Datasource should not be null");
        }
        assertTrue(configuration.connectionFactoryConfiguration().jdbcUrl().contains("jdbc:postgresql:"));
-        assertEquals("quarkus", configuration.connectionFactoryConfiguration().principal().getName());
+        assertEquals("quarku_1", configuration.connectionFactoryConfiguration().principal().getName());


Can't you use the constants? BTW, this is a consequence of the mix you made between username and password. This should be the username here.

This would require adding a dependency.

...yment-spi/src/main/java/io/quarkus/datasource/deployment/spi/DatabaseDefaultSetupConfig.java

newur · 2022-05-06T18:38:16Z

Not sure why this test fails. Works here with JDK 11 when I run it via mvn test.

GavinRay97 · 2022-05-07T17:04:23Z

Would this be a breaking change, for anyone who had manual connection strings specifying the database in their tests before?

newur · 2022-05-07T18:17:16Z

Would this be a breaking change, for anyone who had manual connection strings specifying the database in their tests before?

I would not think so. The whole point of the dev services is that your app does not specify any connection details.

gsmet

I added some suggestions and questions. Could you have a look?

I want to make sure we don't do any mistakes here.

(I think we're nearly there)

gsmet · 2022-05-19T19:47:40Z

...evservices/h2/src/main/java/io/quarkus/devservices/h2/deployment/H2DevServicesProcessor.java

-                            "sa",
-                            "sa",
+                            DEFAULT_DATABASE_USERNAME,
+                            DEFAULT_DATABASE_PASSWORD,


Just checking but we have the full permissions when doing that and not using sa? I thought it had a special meaning? Given I don't see us defining permissions for the quarkus user, I'm wondering how this works?

By default, if the database specified in the embedded URL does not yet exist, a new (empty) database is created automatically. The user that created the database automatically becomes the administrator of this database.

From H2 docu https://h2database.com/html/tutorial.html#creating_new_databases
sa is the abriviation for System Administrator, but seems to have no special permissions. It is just some default IMHO.

...s/mssql/src/main/java/io/quarkus/devservices/mssql/deployment/MSSQLDevServicesProcessor.java

gsmet · 2022-05-19T19:50:24Z

...yment-spi/src/main/java/io/quarkus/datasource/deployment/spi/DatabaseDefaultSetupConfig.java

+    public static final String DEFAULT_DATABASE_USERNAME = "quarkus";
+
+    // mssql container enforces a 'strong' password with min 8 chars, upper/lowercase, number or special char
+    public static final String DEFAULT_DATABASE_PASSWORD = "quarkus1!";


I think I would rather use quarkus for the password and have a STRONG_DEFAULT_DATABASE_PASSWORD for databases that requires it so only MSSQL for now.

Rationale is that in some cases you want to connect to the database while dev mode is running. And it's better to have a simple password if we can.

gastaldi · 2022-05-31T00:55:54Z

This PR needs a rebase since the H2DevServicesProcessor class changed recently

gsmet · 2022-06-21T14:05:29Z

I'm working on this as I want to make other changes in this area and thought it would be better to build on this one.

gsmet

I made a few adjustments for the doc and also added a commit to be able to easily override database names, usernames and passwords.

It's not obvious from the GitHub UI but authorship of the first commit has been preserved: Author: newur <newur@mailbox.org>. My guess is that there's a mismatch between email used in commit and emails declared on GitHub.

gastaldi

CI failures seem related, probably because it needs to use the DEFAULT_DATABASE_STRONG_PASSWORD.

BTW I think the DEFAULT_DATABASE_STRONG_PASSWORD should be used in all databases instead of having just for MSSQL

...s/mssql/src/main/java/io/quarkus/devservices/mssql/deployment/MSSQLDevServicesProcessor.java

gastaldi

CI failures seem related, probably because it needs to use the DEFAULT_DATABASE_STRONG_PASSWORD.

BTW I think the DEFAULT_DATABASE_STRONG_PASSWORD should be used in all databases instead of having just for MSSQL

gastaldi · 2022-06-21T19:18:30Z

How about changing the password as something easier to remember? Like Quarkus#123?

gsmet · 2022-06-21T20:31:23Z

I changed the password to Quarkus123. I think it's enough.

As for using the strong password everywhere, I don't think it's worth making things complicated for the small number of SQL Server users compared to the users of all the other databases.

Not all database containers support everything but it's implemented for all everything that is supported.

gsmet · 2022-06-22T09:50:07Z

I merged it, it's a step in the right direction. Thanks @newur for starting this work!

gsmet requested changes May 2, 2022

View reviewed changes

newur force-pushed the unify-devservices-datasource-names branch from 6cdefc9 to 0687d78 Compare May 2, 2022 09:57

newur requested a review from gsmet May 2, 2022 09:59

This comment has been minimized.

Sign in to view

gsmet force-pushed the unify-devservices-datasource-names branch from 0687d78 to 9631bfb Compare May 3, 2022 11:55

This comment has been minimized.

Sign in to view

quarkus-bot bot added the area/agroal label May 3, 2022

This comment has been minimized.

Sign in to view

quarkus-bot bot added the area/persistence label May 4, 2022

This comment has been minimized.

Sign in to view

newur force-pushed the unify-devservices-datasource-names branch from 92194db to 9ac7195 Compare May 4, 2022 21:40

gsmet requested changes May 5, 2022

View reviewed changes

newur force-pushed the unify-devservices-datasource-names branch 2 times, most recently from 137fb26 to 2d6f47a Compare May 6, 2022 06:16

This comment has been minimized.

Sign in to view

newur force-pushed the unify-devservices-datasource-names branch from 2d6f47a to 9e606d6 Compare May 6, 2022 16:32

This comment has been minimized.

Sign in to view

gsmet reviewed May 19, 2022

View reviewed changes

newur force-pushed the unify-devservices-datasource-names branch from 9e606d6 to ec77b00 Compare May 20, 2022 16:15

quarkus-bot bot added the area/documentation label May 20, 2022

This comment has been minimized.

Sign in to view

newur requested a review from gsmet May 28, 2022 06:17

gastaldi added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label May 31, 2022

gastaldi assigned gsmet Jun 21, 2022

gsmet changed the title ~~Unify devservices datasource names~~ Unify devservices datasource names and allow database, username and password configuration Jun 21, 2022

gsmet changed the title ~~Unify devservices datasource names and allow database, username and password configuration~~ Unify devservices datatabase, username and password and allow configuration Jun 21, 2022

gsmet added release/breaking-change release/noteworthy-feature and removed triage/needs-rebase This PR needs to be rebased first because it has merge conflicts labels Jun 21, 2022

newur and others added 2 commits June 21, 2022 17:03

Unify Dev Services datasource names

b63f784

Some documentation improvements around Database Dev Services unification

8a0fe37

gsmet force-pushed the unify-devservices-datasource-names branch from 24b78ff to aee2dd8 Compare June 21, 2022 15:19

gsmet approved these changes Jun 21, 2022

View reviewed changes

gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 21, 2022

This comment has been minimized.

Sign in to view

gastaldi reviewed Jun 21, 2022

View reviewed changes

...s/mssql/src/main/java/io/quarkus/devservices/mssql/deployment/MSSQLDevServicesProcessor.java Outdated Show resolved Hide resolved

...s/mssql/src/main/java/io/quarkus/devservices/mssql/deployment/MSSQLDevServicesProcessor.java Outdated Show resolved Hide resolved

gastaldi reviewed Jun 21, 2022

View reviewed changes

Allow easily overriding database, username and password for Dev Services

c7b90ee

Not all database containers support everything but it's implemented for all everything that is supported.

gsmet force-pushed the unify-devservices-datasource-names branch from aee2dd8 to c7b90ee Compare June 21, 2022 20:33

gsmet merged commit fe1c236 into quarkusio:main Jun 22, 2022

quarkus-bot bot added this to the 2.11 - main milestone Jun 22, 2022

quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify devservices datatabase, username and password and allow configuration #25288

Unify devservices datatabase, username and password and allow configuration #25288

newur commented May 1, 2022

gsmet left a comment

gsmet commented May 2, 2022

This comment has been minimized.

gsmet commented May 3, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

newur commented May 4, 2022

gsmet left a comment

gsmet May 5, 2022

newur May 5, 2022

This comment has been minimized.

This comment has been minimized.

newur commented May 6, 2022

GavinRay97 commented May 7, 2022

newur commented May 7, 2022

gsmet left a comment

gsmet May 19, 2022

newur May 20, 2022

gsmet May 19, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gastaldi commented May 31, 2022

gsmet commented Jun 21, 2022

gsmet left a comment

This comment has been minimized.

gastaldi left a comment

gastaldi left a comment

gastaldi commented Jun 21, 2022

gsmet commented Jun 21, 2022

gsmet commented Jun 22, 2022

Unify devservices datatabase, username and password and allow configuration #25288

Unify devservices datatabase, username and password and allow configuration #25288

Conversation

newur commented May 1, 2022

gsmet left a comment

Choose a reason for hiding this comment

gsmet commented May 2, 2022

This comment has been minimized.

gsmet commented May 3, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

newur commented May 4, 2022

gsmet left a comment

Choose a reason for hiding this comment

gsmet May 5, 2022

Choose a reason for hiding this comment

newur May 5, 2022

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

newur commented May 6, 2022

GavinRay97 commented May 7, 2022

newur commented May 7, 2022

gsmet left a comment

Choose a reason for hiding this comment

gsmet May 19, 2022

Choose a reason for hiding this comment

newur May 20, 2022

Choose a reason for hiding this comment

gsmet May 19, 2022

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gastaldi commented May 31, 2022

gsmet commented Jun 21, 2022

gsmet left a comment

Choose a reason for hiding this comment

This comment has been minimized.

gastaldi left a comment

Choose a reason for hiding this comment

gastaldi left a comment

Choose a reason for hiding this comment

gastaldi commented Jun 21, 2022

gsmet commented Jun 21, 2022

gsmet commented Jun 22, 2022