-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move OidcTenantConfig out of the runtime package #8596
Move OidcTenantConfig out of the runtime package #8596
Conversation
eed9a7a
to
c839ca6
Compare
|
c839ca6
to
59c5ce3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably have a specific object to build this configuration when multi-tenancy is being used. It would help to improve encapsulation and remove those getters/setters that are kind of redundant now.
I'm approving because I think you mentioned we are going to handle this in the future. But the way it is now users can set config properties in different ways when using multi-tenancy.
@pedroigor Hi, you had an idea awhile back that we can introduce a builder API, I might give it a go later. |
@pedroigor @gsmet I keep getting a resteasy deployment test failure, Security2 tests pass: I'll rebase again just to try to get a total green, but will merge, unless there will be more review comments if the same reasteasy test fails, as it has to make it into 1.4.0.Final if it to make it at all :-) |
59c5ce3
to
108ed93
Compare
The same resteasy deployment failure, so I'll just go ahead or else it won't make it at all. |
@gsmet Can you explain why it is not backported ? I've updated the 1.4 migration guide thinking it was already backported (or will be shortly) |
@gsmet OK, got it. Sorry I missed that the backport was already done, could've checked |
Fixes #8347
I can update 1.4.0.Final release notes and also sent the note to the forum once this PR is merged. CC @gsmet