Repository navigation
fix: MigrateTo26_6_0 should only add organization step to built-in br… - #47991
Conversation
f1a2260 to
f8f245f
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.model.session.UserSessionProviderModelTest#testStreamsMarshallingKeycloak CI - Store Model Tests |
d080a0b to
1190791
Compare
|
@maeventer Thanks for the fix. I would expect users to manually fix this because it would be hard to fix custom browser flows without making this worse. |
|
@maeventer, I will add an upgrade note to warn people about how to fix. |
|
@pedroigor There's potentially another level here: If I had a realm with a bound flow, the old migration will not have updated that realm's default browser flow with the alias "browser", but only the bound flow. If they then switch to the built-in flow (which they can't edit through API), they won't have the expected state in the "browser" flow, and future migrations that may expect all realms to have the same structure won't find that structure, and fail then. |
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.model.session.UserSessionProviderModelTest#testStreamsMarshallingKeycloak CI - Store Model Tests |
There was a problem hiding this comment.
I assume this migration needs to be renamed to MigrateTo26_6_1.java and scheduled for that patch release as otherwise it will not re-run on any installation that already update to 26.6.0
|
Is renaming it enough, or should we keep it around and copy it? Because if it's only renamed, in addition to "fixing" the non-renamed ones, everyone who has not yet upgraded to 26.6.0, but does upgrade to 26.6.1, will still get their custom bound flow broken by the existing migration? |
Maybe "renaming" wasn't a good word. Let me describe it again: Once the migration is fixed, and its
Please let me know if you disagree. |
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.model.singleUseObject.SingleUseObjectModelTest#testClusterKeycloak CI - Store Model Tests |
07b54a8 to
3354d52
Compare
|
@pedroigor - please review my latest changes:
|
The only thing is that in the case you upgraded to 26.6.0 the added executions in your custom browser flow will have set builtIn to true what makes it quite hard to simply delete it via the admin api, maybe it's enough then to copy the own flow again and manipulate it there? |
|
So it would require users to set the organization subflows/executions to not built-in on the database level, and then delete it via admin console/admin API, because deleting a flow that has built-in subflows/executions via the admin console/API is not supported. Maybe it's worth adding that part to the change note, so users aren't surprised by it? |
|
I think you are right about the problem with the removal of a flow with WDYT? Does it sound simpler and less messy than touching the DB or creating a new copy flows, updating all the connected bit and pieces and leaving some garbage flows around that cannot be deleted? |
Would it be an option to allow deleting sub-flows if a flow is not a built-in flow? |
|
Thanks, @ahus1. I think the remaining concern is how people can now (easily) update their custom flows? |
@pedroigor - yes, I think so although I'm not so familiar with those flows. Can we clean up the existing custom flows to not contain the build-in marker? Are sub-flows shared between flows? I have no knowledge in that area, sorry. Waiting for your guidance how how to proceed. |
Sub-flows are not shared. We are adding a subflow to the browser flow, basically. |
3354d52 to
069f6cd
Compare
|
@maeventer @sonOfRa @ahus1 @martin-kanis Updated the PR to allow removing sub-flows and executions whenever the top-level flow (authentication flow) is not a built-in flow. It should be enough to allow people to remove the organization (subflow) entirely from their custom flows. |
@pedroigor thats the best solution in my point of view, thank you very much |
069f6cd to
3ab5817
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.model.singleUseObject.SingleUseObjectModelTest#testClusterKeycloak CI - Store Model Tests |
…owser flow Closes keycloak#47908 Signed-off-by: maeberl <markus.eberl@eventim.de>
Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>
…n flow Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
3ab5817 to
0f67543
Compare
|
Rebased due to release notes conflict. |
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.model.singleUseObject.SingleUseObjectModelTest#testClusterKeycloak CI - Store Model Tests |
|
@pedroigor - looks good for me now, but I needed to resolve some conflicts with the release notes. CI also seems to be unstable. Can you continue to watch it? Once it looks good to you, please go ahead and merge. |
There was a problem hiding this comment.
LGTM. I only reviewed, but did not tested, so relying also on the review by @pedroigor and @ahus1
|
Thanks, @maeventer. |
Closes #47908
This fix prevents the issue from occurring on fresh migrations, but what about realms that have already been incorrectly migrated by 26.6.0?
Should we add a follow-up migration (e.g. MigrateTo26_6_1 or similar) that detects and cleans up the broken state — specifically, removing the injected built-in Organization sub-flow from non-built-in browser flows? Or is the expectation that
affected users fix this manually?