Repository navigation
fix: converting db-connect-timeout to be used for all datasources - #48055
Conversation
closes: keycloak#48053 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
There was a problem hiding this comment.
Pull request overview
Extends --db-connect-timeout support to named datasources (e.g. --db-connect-timeout-users) and consolidates the vendor-specific timeout mapping logic in the Quarkus runtime configuration layer.
Changes:
- Adds
db-connect-timeout-<datasource>as a generated datasource option and exposes it in CLI help output. - Refactors connect-timeout mapping in
DatabasePropertyMappersto use a consolidated vendor-aware mapper. - Updates/extends runtime configuration tests and CLI “unknown option” suggestions accordingly.
Reviewed changes
Copilot reviewed 5 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/DatabasePropertyMappers.java | Refactors/connects db-connect-timeout mapping and enables it for wildcard (named) datasources. |
| quarkus/config-api/src/main/java/org/keycloak/config/DatabaseOptions.java | Adds DB_CONNECT_TIMEOUT to the list of options that get -<datasource> variants. |
| quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/DatasourcesConfigurationTest.java | Adds assertions for connect-timeout behavior with named datasources. |
| quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/ConfigurationTest.java | Updates default timeout tests to align with the new consolidated mapping behavior. |
| quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/cli/PicocliTest.java | Updates “Possible solutions” list to include --db-connect-timeout-<datasource>. |
| quarkus/tests/integration/src/test/resources/org/keycloak/it/cli/dist/approvals/cli/help/HelpCommandDistTest.testUpdateCompatibilityMetadataHelpAll.approved.txt | CLI help snapshot updated to include --db-connect-timeout-<datasource>. |
| quarkus/tests/integration/src/test/resources/org/keycloak/it/cli/dist/approvals/cli/help/HelpCommandDistTest.testUpdateCompatibilityMetadataHelp.approved.txt | CLI help snapshot updated to include --db-connect-timeout-<datasource>. |
| quarkus/tests/integration/src/test/resources/org/keycloak/it/cli/dist/approvals/cli/help/HelpCommandDistTest.testUpdateCompatibilityCheckHelpAll.approved.txt | CLI help snapshot updated to include --db-connect-timeout-<datasource>. |
| quarkus/tests/integration/src/test/resources/org/keycloak/it/cli/dist/approvals/cli/help/HelpCommandDistTest.testUpdateCompatibilityCheckHelp.approved.txt | CLI help snapshot updated to include --db-connect-timeout-<datasource>. |
| quarkus/tests/integration/src/test/resources/org/keycloak/it/cli/dist/approvals/cli/help/HelpCommandDistTest.testStartOptimizedHelpAll.approved.txt | CLI help snapshot updated to include --db-connect-timeout-<datasource>. |
| quarkus/tests/integration/src/test/resources/org/keycloak/it/cli/dist/approvals/cli/help/HelpCommandDistTest.testStartOptimizedHelp.approved.txt | CLI help snapshot updated to include --db-connect-timeout-<datasource>. |
| quarkus/tests/integration/src/test/resources/org/keycloak/it/cli/dist/approvals/cli/help/HelpCommandDistTest.testStartHelpAll.approved.txt | CLI help snapshot updated to include --db-connect-timeout-<datasource>. |
| quarkus/tests/integration/src/test/resources/org/keycloak/it/cli/dist/approvals/cli/help/HelpCommandDistTest.testStartHelp.approved.txt | CLI help snapshot updated to include --db-connect-timeout-<datasource>. |
| quarkus/tests/integration/src/test/resources/org/keycloak/it/cli/dist/approvals/cli/help/HelpCommandDistTest.testStartDevHelpAll.approved.txt | CLI help snapshot updated to include --db-connect-timeout-<datasource>. |
| quarkus/tests/integration/src/test/resources/org/keycloak/it/cli/dist/approvals/cli/help/HelpCommandDistTest.testStartDevHelp.approved.txt | CLI help snapshot updated to include --db-connect-timeout-<datasource>. |
| quarkus/tests/integration/src/test/resources/org/keycloak/it/cli/dist/approvals/cli/help/HelpCommandDistTest.testImportHelpAll.approved.txt | CLI help snapshot updated to include --db-connect-timeout-<datasource>. |
| quarkus/tests/integration/src/test/resources/org/keycloak/it/cli/dist/approvals/cli/help/HelpCommandDistTest.testImportHelp.approved.txt | CLI help snapshot updated to include --db-connect-timeout-<datasource>. |
| quarkus/tests/integration/src/test/resources/org/keycloak/it/cli/dist/approvals/cli/help/HelpCommandDistTest.testExportHelpAll.approved.txt | CLI help snapshot updated to include --db-connect-timeout-<datasource>. |
| quarkus/tests/integration/src/test/resources/org/keycloak/it/cli/dist/approvals/cli/help/HelpCommandDistTest.testExportHelp.approved.txt | CLI help snapshot updated to include --db-connect-timeout-<datasource>. |
| quarkus/tests/integration/src/test/resources/org/keycloak/it/cli/dist/approvals/cli/help/HelpCommandDistTest.testBootstrapAdminUser.approved.txt | CLI help snapshot updated to include --db-connect-timeout-<datasource>. |
| quarkus/tests/integration/src/test/resources/org/keycloak/it/cli/dist/approvals/cli/help/HelpCommandDistTest.testBootstrapAdminService.approved.txt | CLI help snapshot updated to include --db-connect-timeout-<datasource>. |
| return false; | ||
| } | ||
| String dbDriver = getDatasourceOptionValue(DatabaseOptions.DB_DRIVER, datasource).orElse(null); | ||
| if (!Objects.equals(Database.getDriver(db, true).orElse(null), dbDriver) && |
There was a problem hiding this comment.
Should we drop this check? It annoys me that the CLI options only apply to default drivers. cc @ahus1
There was a problem hiding this comment.
@pruivo do you want to address that here or in a follow-up?
There was a problem hiding this comment.
@shawkins If you agree with the suggestion, do it here 👍
There was a problem hiding this comment.
My understanding of the concern is that an alternative driver, such as datadirect, may not use the same property key/value semantics as the standard one, so it's better to do nothing than something that may be wrong.
Co-authored-by: Martin Bartoš <mabartos@redhat.com> Signed-off-by: Steven Hawkins <shawkins@redhat.com>
…ycloak#48055) * fix: converting db-connect-timeout to be used for all datasources closes: keycloak#48053 Signed-off-by: Steve Hawkins <shawkins@redhat.com> * Apply suggestions from code review Co-authored-by: Martin Bartoš <mabartos@redhat.com> Signed-off-by: Steven Hawkins <shawkins@redhat.com> --------- Signed-off-by: Steve Hawkins <shawkins@redhat.com> Signed-off-by: Steven Hawkins <shawkins@redhat.com> Co-authored-by: Martin Bartoš <mabartos@redhat.com>
closes: #48053
Also consolidates the connect timeout logic.