-
Notifications
You must be signed in to change notification settings - Fork 961
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
Updating user-storage-jpa quickstart for the new distribution #302
Conversation
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.
Nice! :) Apart from the naming (and thats always a hard problem, but not blocking for this iteration i think ;) ) LGTM
...ge-jpa/src/main/java/org/keycloak/quickstart/storage/user/EjbExampleUserStorageProvider.java
Outdated
Show resolved
Hide resolved
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.
I tried out the example together with the PR enabling multiple data sources and found the following things that surprised me:
- README.md contains "Integration test of the Quickstart" that is obsolete as the integration tests don't work for this Quarkus example, yet. Consider removing that part, and also the tests from the project.
- README.md references Java EE in the section "More Information", that is probably out of date?
- I can create new users and log in with these users via the UI, still I can't search for these new users I added (neither by keyword, nor by selecting "View all users"). If this is expected, please add this to the README.md
- The database setup uses an in-memory database that will drop all data once the last connection has been closed (
jdbc:h2:mem:user-store
), even when Keycloak is still running. To avoid this, append;DB_CLOSE_DELAY=-1
to the DB URL; this will prevent deleting the contents. Otherwise Keycloak will forget about the data after a few idle minutes. - Mention in the README.md the minimum Keycloak version where this will work (Keycloak 18-SNAPSHOT), to prevent others from trying this with Keycloak 17.
- Please create a new issue to add an integration test for this quick-start to ensure that it continues to work with new versions of Keycloak.
...-legacy/src/main/resources/META-INF/services/org.keycloak.storage.UserStorageProviderFactory
Outdated
Show resolved
Hide resolved
@ahus1 thanks for trying it out! just double-checked and you are right, login / creation works (that's what i have tested) but search does not. Also not sure if it should, but guess so. |
@ahus1 Thanks for the review. Updated the PR with most of your suggestions. What is missing is bellow:
Tests were removed for now and we have another issue to change this repo to run against the new distribution. See #287.
I'm able to query users by keyword and the behavior when querying all users and not showing those from the provider is the same as for the legacy distribution. I'm keeping the same instructions from legacy.
I don't think we should include this as it is going to become obsolete very soon.
As previously mentioned, see #287. |
07edd5f
to
0a6fe93
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.
Changes are good for now and show a minimal working provider.
@dasniko Thanks. Applied the changes. |
@@ -0,0 +1,3 @@ | |||
quarkus.datasource.user-store.db-kind=h2 |
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.
@pedroigor while I am reworking the quarkus properties behaviour i noticed that the standard transaction mode is "enabled" instead of "xa" as it is for us. I am not sure if we should advertise to set transaction mode to XA here, too ( quarkus.datasource.user-store.transactions=xa
), so wanted to ask if needed
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.
@pedroigor @stianst afaik this one should've been merged a while ago, no? Was irritated bc of #314 but seen this one isn't merged yet. So imo we should merge.
No description provided.