8000 fix: converting db-connect-timeout to be used for all datasources by shawkins · Pull Request #48055 · keycloak/keycloak · GitHub
[go: up one dir, main page]

Skip to content

fix: converting db-connect-timeout to be used for all datasources - #48055

Merged
mabartos merged 2 commits into
keycloak:mainfrom
shawkins:iss48053
Apr 16, 2026
Merged

mabartos merged 2 commits into
keycloak:mainfrom
shawkins:iss48053

Conversation

@shawkins
Copy link
Copy Markdown
Contributor

closes: #48053

Also consolidates the connect timeout logic.

closes: keycloak#48053

Signed-off-by: Steve Hawkins <shawkins@redhat.com>
@shawkins
shawkins marked this pull request as ready for review April 14, 2026 20:07
@shawkins
shawkins requested review from a team as code owners April 14, 2026 20:07
@shawkins
shawkins requested a review from a team April 14, 2026 20:07
@mabartos
mabartos requested a review from Copilot April 15, 2026 05:03
Copilot AI left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 DatabasePropertyMappers to 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>.

@mabartos mabartos left a comment
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@shawkins Just a few suggestions for the named datasource keys for URL and properties (even as Copilot suggested). Otherwise LGTM, nice! Would be good to update/add tests for this, as it hasn't been caught.

return false;
}
String dbDriver = getDatasourceOptionValue(DatabaseOptions.DB_DRIVER, datasource).orElse(null);
if (!Objects.equals(Database.getDriver(db, true).orElse(null), dbDriver) &&
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we drop this check? It annoys me that the CLI options only apply to default drivers. cc @ahus1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@pruivo do you want to address that here or in a follow-up?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@shawkins If you agree with the suggestion, do it here 👍

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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>
@shawkins
shawkins requested a review from mabartos April 15, 2026 12:36
@mabartos mabartos left a comment
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@shawkins LGTM, thanks!

@mabartos
mabartos merged commit 71bdc5e into keycloak:main Apr 16, 2026
88 checks passed
@stianst stianst mentioned this pull request Jun 1, 2026
sashyo pushed a commit to tide-foundation/keycloak-IGA that referenced this pull request Jul 20, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow db-connect-timeout to be used for all datasources

4 participants

0