Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
Updates empty-database initialization defaults to respect manual and validation migration strategies.
Changes:
- Makes
initialize-emptystrategy-dependent while preserving explicit overrides. - Adds distribution tests for manual and validation behavior.
- Documents the behavioral change.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
QuarkusJpaConnectionProviderFactory.java |
Applies strategy-dependent initialization defaults. |
DefaultJpaConnectionProviderFactory.java |
Mirrors behavior in the legacy provider. |
EmptyDatabaseMigrationStrategyDistTest.java |
Tests empty-database migration scenarios. |
changes-26_8_0.adoc |
Adds the upgrade notice. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
aafcf42 to
7747c79
Compare
7747c79 to
c015df1
Compare
| Breaking changes are identified as those that might require changes for existing users to their configurations or applications. | ||
| In minor or patch releases, {project_name} will only introduce breaking changes to fix bugs. | ||
|
|
||
| === Empty database and an explicit migration strategy |
There was a problem hiding this comment.
AFAICT when user configures update migration strategy, this initialize-empty has no effect. Regardless of true or false or unset. Should documentation mention it or it is already doc-ed or implied somehow?
There was a problem hid 8000 ing this comment.
You're right @michalvavrik , with update an empty database is always initialized, regardless of initialize-empty. That was already the case before this change; both the initializeEmpty branch and the UPDATE case of the switch end up in update().
It isn't documented anywhere though, and the help text of initializeEmpty ("If set to false the database has to be manually initialized") is actually misleading for update. I'd add one sentence to this upgrade note and fix the help text:
initialize-emptyonly affects themanualandvalidatestrategies. Withupdate, an empty database is always initialized regardless of this setting.
Want me to push that here, or would you rather keep it out of this PR?
There was a problem hiding this comment.
I just thought we should document it somehow, I understand it is preexisting "fact". But since you are fixing the initialize-empty, doc-ing this would be good. If you don't mind, do it. As I say these days "short concise note will do" 😁 Thanks
There was a problem hiding this comment.
Done, short and concise as requested :) One sentence in the upgrade note plus the help text, and rebased onto main. Thanks!
c015df1 to
b238af2
Compare
| @@ -0,0 +1,85 @@ | |||
| /* | |||
| * Copyright 2026 Red Hat, Inc. and/or its affiliates | |||
There was a problem hiding this comment.
keep it, just fyi: it is not necessary anymore to add this
b238af2 to
3ecb184
Compare
3ecb184 to
4089be9
Compare
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.broker.KcSamlBrokerTest#loginInResponseToEmptyKeycloak CI - FIPS IT (strict) org.keycloak.testsuite.broker.KcSamlSignedBrokerTest#loginUserAllNamespacesInTopElementKeycloak CI - FIPS IT (strict) org.keycloak.testsuite.broker.KcSamlSignedBrokerTest#testSignatureDataWhenWantsRequestsSignedKeycloak CI - FIPS IT (strict) |
4089be9 to
4e463f9
Compare
On an empty database initializeEmpty defaults to true and is checked before the migration strategy, so an explicitly configured manual or validate strategy was silently ignored: manual initialized the schema in place without writing the export script, and validate created the whole schema on a database it was told to only validate. Make the default of initialize-empty depend on the strategy instead: true for update, false for manual and validate. An explicitly configured value still wins in all cases, so existing setups keep their behavior and manual plus an explicit initialize-empty=true remains available as an opt-in. Closes keycloak#51932 Signed-off-by: Ali Tugrul Pinar <ali@keymate.io>
4e463f9 to
87cd9ef
Compare
Closes #51932
What
An explicitly configured
migration-strategyis silently ignored when the database is empty:initializeEmptydefaults totrueand is checked before the strategy, somanualinitializes the schemain place without writing the export script, and
validatecreates the whole schema on a database it was toldto only validate. Follow-up requested in
#51730 (comment).
How
The default of
initialize-emptynow depends on the strategy:trueforupdate(unchanged),falseformanualandvalidate. An explicitly configured value wins in all cases:update(no flag)manual(no flag)validate(no flag)manual/validate+ explicitinitialize-empty=truemanual+ explicitinitialize-empty=falseOne line of logic in
QuarkusJpaConnectionProviderFactoryand the same in the legacyDefaultJpaConnectionProviderFactory, plus theinitialize-emptyhelp text and an entry in the upgradingguide, since the two fixed rows are a behavior change.
Note that
initialize-empty=falsecombined withupdatealready updates today, so the option only ever hadan effect on
manualandvalidate; this change gives it a consistent meaning instead of a contradicting one.Testing
New
EmptyDatabaseMigrationStrategyDistTest(H2, raw dist, empty database):manualalone produces the export script and the "Database not initialized" startup error (red without the fix);validatealone fails startup with "please enable database initialization" (red without the fix);manualwith an explicitinitialize-empty=truestill initializes and starts (green before and after, pins the opt-in).Existing manual-strategy tests (
ManualMigrationCustomProviderDistTest,LiquibaseDistTest,PostgreSQLDistTest/MySQLDistTest/MariaDBDistTest) all passinitialize-emptyexplicitly, so theirbehavior is unchanged; the first two were re-run locally and are green.
Note on AI assistance
AI agents were used while investigating this and while drafting the change, tests and documentation. I have
reviewed and understand every line, ran the verification described above myself, and will handle review
feedback directly.