Skip to content

fix: addressing manual migration issues due to TEST - #54580

Merged
shawkins merged 2 commits into
keycloak:mainfrom
shawkins:iss54404
Nov 11, 2026
Merged

fix: addressing manual migration issues due to TEST#54580
shawkins merged 2 commits into
keycloak:mainfrom
shawkins:iss54404

Conversation

@shawkins

@shawkins shawkins commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

the usage of TEST in manual mode can lead to inconsistent scripts if the preconditions or task logic are relying on existing state

closes: #54404

@shawkins
shawkins marked this pull request as ready for review November 3, 2026 14:26
Copilot AI balanced review requested due to automatic review settings November 3, 2026 14:26
@shawkins
shawkins requested review from a team as code owners November 3, 2026 14:26

This comment was marked as outdated.

Sign in to view

Copilot AI balanced review requested due to automatic review settings November 3, 2026 14:56

This comment was marked as outdated.

Sign in to view

keycloak-github-bot[bot]
keycloak-github-bot Bot reviewed Nov 3, 2026
View reviewed changes

@keycloak-github-bot keycloak-github-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@keycloak-github-bot

keycloak-github-bot Bot commented Nov 3, 2026

Copy link
Copy Markdown

Unreported flaky test detected

If 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.adapter.servlet.SAMLServletAdapterTest#salesPostEncTest

Keycloak CI - Adapter IT

org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
...

Report flaky test

Copilot AI balanced review requested due to automatic review settings November 3, 2026 17:49

This comment was marked as outdated.

Sign in to view

Copilot AI balanced review requested due to automatic review settings November 3, 2026 20:23

This comment was marked as outdated.

Sign in to view

Copilot AI balanced review requested due to automatic review settings November 3, 2026 20:42
@shawkins
shawkins marked this pull request as draft November 3, 2026 20:42

This comment was marked as outdated.

Sign in to view

@shawkins
shawkins requested a balanced review from Copilot November 3, 2026 21:28

This comment was marked as outdated.

Sign in to view

@shawkins
shawkins requested a balanced review from Copilot November 4, 2026 15:27
Copilot AI reviewed Nov 4, 2026
View reviewed changes

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.

🔵 Needs a closer look

The shared Liquibase migrations need runtime validation of populated upgrades across supported databases, since the new parity tests cover only empty databases and do not verify organization data preservation or deferred-index upgrades.

No open issues 🎉

Review effort: Balanced

@shawkins
shawkins marked this pull request as ready for review November 4, 2026 15:34
@shawkins

shawkins commented Nov 4, 2026

Copy link
Copy Markdown
Contributor Author

The shared Liquibase migrations need runtime validation of populated upgrades across supported databases, since the new parity tests cover only empty databases and do not verify organization data preservation or deferred-index upgrades.

I have tests for that locally, but they seem fairly large for the amount of coverage that they provide - they require for example quite a lot of setup to put the server into a pre-26.8 state with the exact organization data that would highlight the issue.

@shawkins
shawkins requested review from ahus1 and vmuzikar November 4, 2026 15:36
vmuzikar
vmuzikar reviewed Nov 6, 2026
View reviewed changes

@vmuzikar vmuzikar 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.

As far as I can tell, overall LGTM. But I'd like to get one more review at least. Maybe also @vramik could give it a look as he's the author of the original change sets?

Comment thread docs/documentation/upgrading/topics/changes/changes-27_0_0.adoc Resolved
@vramik
vramik self-requested a review November 6, 2026 19:47
@shawkins
fix: addressing manual migration issues due to TEST
cf03301 
closes: keycloak#54404

Signed-off-by: Steve Hawkins <shawkins@redhat.com>
@shawkins
adding a more comprehensive check of manual migrations
6e147f7 
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
Copilot AI balanced review requested due to automatic review settings November 9, 2026 17:54
Copilot AI reviewed Nov 9, 2026
View reviewed changes

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.

🔵 Needs a closer look

Populated upgrades need cross-database runtime validation because the new empty-database tests skip organization backfills and do not cover persisted-history preconditions or threshold-skipped indexes.

No open issues 🎉

Review effort: Balanced

vmuzikar
vmuzikar approved these changes Nov 10, 2026
View reviewed changes

@vmuzikar vmuzikar 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.

LGTM but still I'd be happy to get a review from @vramik ;)

@shawkins

shawkins commented Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

@ahus1 do you want to take a look as well?

ahus1
ahus1 reviewed Nov 11, 2026
View reviewed changes

@ahus1 ahus1 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.

Hello @shawkins, thanks for investigating this rabbit hole!

I'm confident that the changes you did are valid. I recently added the index checker that still works (based on its own test), and also does not show and indexes created afterwards after a fresh startup.

So I assume all index changes are valid.

Looking through the code, I see a lot of tests. I assume testScriptProducesTheSchemaOfTheAutomaticMigration will prevent future problems?

It might be worth adding a readme on best practices / how to write changelogs correctly either here or in a follow-up.

OK to merge.

@shawkins

shawkins commented Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

Looking through the code, I see a lot of tests. I assume testScriptProducesTheSchemaOfTheAutomaticMigration will prevent future problems?

Yes when starting from an empty database. Since that is h2 specific, similar testing exists in the database specific dist tests.

@shawkins
shawkins merged commit 9da3ba7 into keycloak:main Nov 11, 2026
104 checks passed
@shawkins shawkins mentioned this pull request Nov 11, 2026
Open
@shawkins

shawkins commented Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

@ahus1 created #55057 as a follow-up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahus1 ahus1 ahus1 left review comments
Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@vmuzikar vmuzikar vmuzikar approved these changes
+2 more reviewers
@keycloak-github-bot keycloak-github-bot[bot] keycloak-github-bot[bot] left review comments
@vramik vramik vramik approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

flaky-test team/cloud-native

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Manual migration export on an empty database fails: IDX_USER_SESSION_EXPIRATION_CREATED is re-created without the old index being dropped (26.8.0)

5 participants

@shawkins @vramik @ahus1 @vmuzikar