8000 use logged in realm as default by edewit · Pull Request #22460 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

use logged in realm as default - #22460

Merged
ssilvert merged 1 commit into
keycloak:mainfrom
edewit:default-value
Aug 16, 2023
Merged

use logged in realm as default#22460
ssilvert merged 1 commit into
keycloak:mainfrom
edewit:default-value

Conversation

@edewit
@edewit edewit commented Aug 15, 2023
Copy link
Copy Markdown
Contributor

No description provided.

@edewit
8000
edewit requested a review from a team as a code owner August 15, 2023 11:26
agagancarczyk
agagancarczyk approved these changes Aug 15, 2023
View reviewed changes
@agagancarczyk agagancarczyk 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!

@ahus1
ahus1 self-requested a review August 15, 2023 12:03
ahus1
ahus1 reviewed Aug 15, 2023
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.

I see that this make the realm parameter introduced in #21554 optional, still it wouldn't make it backward compatible as it previously iterated over all realms.

I don't see a parent issue linked here, so could you please share more about the motivation of this change and the situation where using the current realm would be the right thing to do?

@ssilvert
ssilvert commented Aug 16, 2023
Copy link
Copy Markdown
Contributor

I see that this make the realm parameter introduced in #21554 optional, still it wouldn't make it backward compatible as it previously iterated over all realms.

I don't see a parent issue linked here, so could you please share more about the motivation of this change and the situation where using the current realm would be the right thing to do?

@ahus1 I believe the original motivation for adding the currentRealm param was to make the whois endpoint performant when you have many, many realms. So if we added back the original logic then that would be broken again.

The reason we want a reasonable default is so that the whois call will not throw an NPE when the currewntRealm is missing. This will also allow the old admin console to run. While the old console is not supported, it is still used by development for comparison purposes and potentially by others in the community.

ahus1
ahus1 approved these changes Aug 16, 2023
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.

This will also allow the old admin console to run.

Thank you @ssilvert for this explanation. This makes sense, although I am quite sure that the old admin console would depend on the original logic which wasn't added.

I'm approving this PR, so you can merge this PR you think this is sufficient to run the old Admin console.

@ssilvert
ssilvert merged commit b4650b7 into keycloak:main Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahus1 ahus1 ahus1 approved these changes
+1 more reviewer
@agagancarczyk agagancarczyk agagancarczyk approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@edewit @ssilvert @ahus1 @agagancarczyk
0