Repository navigation
fix: combining / removing static and initialization logic - #46918
Conversation
|
@pedroigor I recall we spoke some time ago about rationalizing the keycloak initialization logic more. This pr, #46378 and some follow-up work should hopefully clean things up a bit. It may even be possible to eventually have the provider logic in quarkus be based upon CDI - I think that would open up another level of quarkus tests for us without the need to bring up the full server. |
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.UserSessionPersisterProviderTest#testExpirationDistributionKeycloak CI - Store Model Tests org.keycloak.testsuite.model.session.UserSessionProviderModelTest#testStreamsMarshallingKeycloak CI - Store Model Tests org.keycloak.testsuite.model.session.OfflineSessionPersistenceTest#testPersistenceMultipleNodesClientSessionAtSameNodeKeycloak CI - Store Model Tests org.keycloak.testsuite.model.session.UserSessionPersisterProviderTest#testUserRemovedKeycloak CI - Store Model Tests |
|
@shawkins - thank you for letting the other PR go in first. Your PR now needs a conflict resolution :( |
ace1c3f to
9543645
Compare
closes: keycloak#46917 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
No problem, it should be good to look at again. @pruivo based upon my previous changes it seemed simpler to just remove the parameterization of the session factory - let me know if you need that for something else. There will be a few more static refinements coming after #47077 |
Sure. I only added the parameterization because the |
There was a problem hiding this comment.
It looks good to me. Thank you.
|
Given the close 26.6 release, I'd like to wait with merging this one. It's a bigger change and "just" an enhancement, still bringing some potential regressions. @shawkins Are you ok with putting this on hold for a bit? |
I'm not sure it needs to be formally put on hold just yet - unless you think it has sufficient reviews. In which case, can you identify specific concerns so the changes could be split into what would be acceptable? Ideally I'd like to address #47077 as well, and at the very least I'd like all the wiring of our session factory within quarkus to be correct. |
My concern are potential regressions. It's not an isolated change (like e.g. the Client v2 changes are) and we're in a "polishing" phase before the release, avoiding potentially risky changes. |
I understand, that's why I was asking for a qualification of what is risky. However if you are saying that we don't want any changes along these lines before 26.6, that's fine too. I was waiting for a review from @pedroigor regardless. |
Signed-off-by: Steven Hawkins <shawkins@redhat.com>
Do you mean introducing a (CDI) bridge between the session factory and making it possible to inject kc-session- (and factory) into tests? Similar to model tests? |
Yes To clarify, near-term goal here is mostly cleanup. The next goal is to make sure that we can use a full application reload in quarkus dev mode such that we're appropriately cleaning up / reinitializing our state (#47077) Tackling a more targeted testing strategy with QuarkusTests would come much later. |
There was a problem hiding this comment.
Much cleaner .... Having the KSF as a bean and moving init logic to their own place improves a lot, plus redundant code when deploying providers at runtime ....
If tests pass, LGTM from me.
…6918) closes: keycloak#46917 Signed-off-by: Steve Hawkins <shawkins@redhat.com> Signed-off-by: Steven Hawkins <shawkins@redhat.com>
closes: #46917