fix: adding a -- separator for spi options by shawkins · Pull Request #40005 · keycloak/keycloak

shawkins · 2025-05-27T19:20:02Z

shawkins · 2025-05-28T11:09:25Z

@vmuzikar are you ok with this being a breaking change from the perspective of users who are relying on auto-builds with -provider, -provider-default, -enabled? If so I can add a note for that.

And / or do you want a warning for now for the possibly ambiguous cases?

keycloak-github-bot · 2025-05-28T11:53:30Z

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.webauthn.account.WebAuthnSigningInTest#checkAuthenticatorTimeLocale

Keycloak CI - WebAuthn IT (chrome)

java.text.ParseException: Unparseable date: "May 28, 2025, 11:26 AM"
	at java.base/java.text.DateFormat.parse(DateFormat.java:399)
	at org.keycloak.testsuite.webauthn.account.WebAuthnSigningInTest.checkAuthenticatorTimeLocale(WebAuthnSigningInTest.java:320)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
...

Report flaky test

keycloak-github-bot

Unreported flaky test detected, please review

vmuzikar

are you ok with this being a breaking change from the perspective of users who are relying on auto-builds with -provider, -provider-default, -enabled? If so I can add a note for that.

Yes, please add a note to the upgrading guide about that. Maybe with some reasoning about it.

And / or do you want a warning for now for the possibly ambiguous cases?

You mean to the docs? Definitely yes. I know you've done some updates to the docs but IMHO we need to be much more explicit. Basically to explain the difference between the two forms and the auto build behaviour. It might very easily get confusing to the users.

Additionally, I think we should update all docs occurences to the -- format.

shawkins · 2025-06-03T12:36:46Z

Updated with a warning when changed the docs to use the new format.

There's still a reference to spi-authentication-sessions-map-auth-sessions-limit (now spi-authentication-sessions--map--auth-sessions-limit) that should be removed correct?

shawkins · 2025-06-03T16:01:14Z

Also updated to internally map to the new spi form - this also highlighted that the PropertyMapper logic needs to maintain an additional mapping to the legacy form.

mabartos

@shawkins Nice work! Using -- separator makes these SPI separations more visible and I like it more than before.

Just added some small comments that might improve things a little bit more IMO.

core/src/main/java/org/keycloak/Config.java

docs/documentation/upgrading/topics/changes/changes-26_3_0.adoc

...untime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMappers.java

shawkins · 2025-06-10T13:11:03Z

@shawkins Nice work! Using -- separator makes these SPI separations more visible and I like it more than before.

Just added some small comments that might improve things a little bit more IMO.

Thank you for the review. Applied the suggestions.

mabartos

@shawkins Looks good, but one last thing - we should move these upgrading notes you provided to the "Notable changes" section and probably not to the "Breaking changes". Or is there anything I missed?

docs/documentation/upgrading/topics/changes/changes-26_3_0.adoc

core/src/main/java/org/keycloak/Config.java

keycloak-github-bot

Unreported flaky test detected, please review

vmuzikar · 2025-06-12T07:17:55Z

I plan to give this a quick look later today.

vmuzikar

@shawkins Sorry for the late review and thank you for the changes!

Overall LGTM but might be good to add some tests for checking the selected SPI options are correctly treated as buildtime.
Additionally, I wonder if we should have a test for the env var form, if __ works as expected.

vmuzikar · 2025-06-12T15:29:40Z

Can you also please rebase?

shawkins · 2025-06-12T15:38:20Z

Overall LGTM but might be good to add some tests for checking the selected SPI options are correctly treated as buildtime.

We already had a test or two along those lines (see StartCommandDistTest.errorSpiBuildtimeAtRuntime for example), and the arquilian logic relies on implicit rebuilds due to provider option changes.

Additionally, I wonder if we should have a test for the env var form, if __ works as expected.

I can certainly add something. The replacement logic in KcEnvConfigSource is now quite straight-forward all _ in a non-wildcard key will become -, so there's nothing special about this case.

Signed-off-by: Steven Hawkins <shawkins@redhat.com>

vmuzikar · 2025-06-12T15:49:50Z

We already had a test or two along those lines (see StartCommandDistTest.errorSpiBuildtimeAtRuntime for example), and the arquilian logic relies on implicit rebuilds due to provider option changes.

But we don't cover all the SPI build time options (i.e. --provider, --enabled and --provider-default), do we?

I can certainly add something. The replacement logic in KcEnvConfigSource is now quite straight-forward all _ in a non-wildcard key will become -, so there's nothing special about this case.

Yeah, it'd be good to have something to make sure that e.g. Quarkus doesn't messes that up somewhere.

shawkins · 2025-06-12T16:09:19Z

But we don't cover all the SPI build time options (i.e. --provider, --enabled and --provider-default), do we?

I'll add something after lunch.

Yeah, it'd be good to have something to make sure that e.g. Quarkus doesn't messes that up somewhere.

Quarkus? These entries are only handled by Keycloak logic.

vmuzikar · 2025-06-12T16:13:27Z

Quarkus? These entries are only handled by Keycloak logic.

Yeah, but I meant some too smart interceptors maybe. Still I'd feel safer with some tests. :)

Signed-off-by: Steve Hawkins <shawkins@redhat.com>

shawkins · 2025-06-12T18:04:16Z

Yeah, but I meant some too smart interceptors maybe. Still I'd feel safer with some tests. :)

See if the the additional changes are sufficient.

vmuzikar

LGTM, thank you.

shawkins · 2025-06-13T13:25:50Z

@mabartos @vmuzikar the rebase was trivial, just wanted to check if either of you want to review again.

mabartos

LGTM

* fix: adding a -- separator for spi options closes: keycloak#39063 Signed-off-by: Steve Hawkins <shawkins@redhat.com> * adding a warning for ambiguous spi options also adding a note about the change Signed-off-by: Steve Hawkins <shawkins@redhat.com> # Conflicts: # docs/documentation/upgrading/topics/changes/changes-26_3_0.adoc * updating docs to the new format Signed-off-by: Steve Hawkins <shawkins@redhat.com> # Conflicts: # docs/guides/high-availability/examples/generated/keycloak-ispn.yaml # docs/guides/high-availability/examples/generated/keycloak.yaml * internally using the new spi options also adding a deprecation notice 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> * correcting options output adding + + inlining where needed Signed-off-by: Steve Hawkins <shawkins@redhat.com> * adding test showing the env mapping with __ Signed-off-by: Steve 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> Signed-off-by: Robin Meese <39960884+robson90@users.noreply.github.com>

pl-me · 2025-08-19T12:56:16Z

@shawkins Hi, I noticed you have changed MicroProfileScope.getPropertyNames() to filter keys using double-dash-separator only. That means it is a breaking change if you rely on getPropertyNames() and still using single-dash options. Am I right?

As getPropertyNames() is now deprecated I guess I should move away from using that function anyway. I just wanted to make sure your change was intentional.

shawkins · 2025-08-19T13:06:43Z

As getPropertyNames() is now deprecated I guess I should move away from using that function anyway. I just wanted to make sure your change was intentional.

The change was intentional, as was marking it as deprecated. Yes you should not rely on this function.

shawkins force-pushed the iss39063 branch 2 times, most recently from cc9ccb0 to 10c501a Compare May 27, 2025 21:10

shawkins marked this pull request as ready for review May 27, 2025 21:11

shawkins requested review from a team as code owners May 27, 2025 21:11

keycloak-github-bot bot added team/cloud-native team/core-clients team/core-iam labels May 27, 2025

shawkins requested a review from vmuzikar May 27, 2025 21:11

shawkins force-pushed the iss39063 branch from 1d1d4d2 to 10c501a Compare May 28, 2025 11:07

keycloak-github-bot bot added the flaky-test label May 28, 2025

keycloak-github-bot bot reviewed May 28, 2025

View reviewed changes

vmuzikar reviewed Jun 3, 2025

View reviewed changes

shawkins force-pushed the iss39063 branch from 7d3fc8e to b4ee5b5 Compare June 3, 2025 12:36

shawkins force-pushed the iss39063 branch 2 times, most recently from bd4744e to ae2a768 Compare June 3, 2025 21:43

mabartos reviewed Jun 10, 2025

View reviewed changes

shawkins force-pushed the iss39063 branch from 4a8d4b6 to a096ece Compare June 10, 2025 13:10

shawkins requested a review from vmuzikar June 10, 2025 13:27

mabartos reviewed Jun 10, 2025

View reviewed changes

docs/documentation/upgrading/topics/changes/changes-26_3_0.adoc Outdated Show resolved Hide resolved

mabartos reviewed Jun 10, 2025

View reviewed changes

core/src/main/java/org/keycloak/Config.java Outdated Show resolved Hide resolved

keycloak-github-bot bot reviewed Jun 11, 2025

View reviewed changes

vmuzikar reviewed Jun 12, 2025

View reviewed changes

Merge branch 'main' into iss39063

2079bc1

Signed-off-by: Steven Hawkins <shawkins@redhat.com>

shawkins dismissed mabartos’s stale review via 2079bc1 June 12, 2025 15:46

adding test showing the env mapping with __

3107ef8

Signed-off-by: Steve Hawkins <shawkins@redhat.com>

shawkins force-pushed the iss39063 branch from 5fbae96 to 3107ef8 Compare June 12, 2025 18:03

vmuzikar previously approved these changes Jun 12, 2025

View reviewed changes

vmuzikar enabled auto-merge (squash) June 12, 2025 18:34

Merge remote-tracking branch 'upstream' into iss39063

2bc82d7

shawkins dismissed vmuzikar’s stale review via 2bc82d7 June 13, 2025 12:35

shawkins requested review from mabartos and vmuzikar June 13, 2025 13:24

mabartos approved these changes Jun 13, 2025

View reviewed changes

vmuzikar approved these changes Jun 13, 2025

View reviewed changes

vmuzikar merged commit 76bc9fa into keycloak:main Jun 13, 2025
80 checks passed

ahus1 mentioned this pull request Jun 17, 2025

Nightly build shows outdated information on the Keycloak website #40542

Closed

2 tasks

vmuzikar mentioned this pull request Jun 24, 2025

fix: correcting additional legacy scope usage #40644

Merged

shawkins mentioned this pull request Aug 5, 2025

Provider default regression #41677

Closed

2 tasks

ryanemerson mentioned this pull request Sep 3, 2025

Document that single-cluster deployments expect all Keycloak instances to serve traffic #42306

Merged

Conversation

shawkins commented May 27, 2025

Uh oh!

shawkins commented May 28, 2025

Uh oh!

keycloak-github-bot bot commented May 28, 2025

Unreported flaky test detected

org.keycloak.testsuite.webauthn.account.WebAuthnSigningInTest#checkAuthenticatorTimeLocale

Uh oh!

keycloak-github-bot bot left a comment

Choose a reason for hiding this comment

Uh oh!

vmuzikar left a comment

Choose a reason for hiding this comment

Uh oh!

shawkins commented Jun 3, 2025

Uh oh!

shawkins commented Jun 3, 2025

Uh oh!

mabartos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shawkins commented Jun 10, 2025

Uh oh!

mabartos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

keycloak-github-bot bot left a comment

Choose a reason for hiding this comment

Uh oh!

vmuzikar commented Jun 12, 2025

Uh oh!

vmuzikar left a comment

Choose a reason for hiding this comment

Uh oh!

vmuzikar commented Jun 12, 2025

Uh oh!

shawkins commented Jun 12, 2025

Uh oh!

vmuzikar commented Jun 12, 2025

Uh oh!

shawkins commented Jun 12, 2025

Uh oh!

vmuzikar commented Jun 12, 2025

Uh oh!

shawkins commented Jun 12, 2025

Uh oh!

vmuzikar left a comment

Choose a reason for hiding this comment

Uh oh!

shawkins commented Jun 13, 2025

Uh oh!

mabartos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pl-me commented Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shawkins commented Aug 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pl-me commented Aug 19, 2025 •

edited

Loading