-
-
Notifications
You must be signed in to change notification settings - Fork 55
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
Add configuration to disable bootstrap of admin account #165
Add configuration to disable bootstrap of admin account #165
Conversation
484105c
to
5488465
Compare
5488465
to
f2510aa
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.
Thanks for this PR, generally LGTM and makes somehow sense with the new behavior.
There are some small issues, mentioned directly at the code, please review and fix it.
Additionally, there is some documentation missing. Please add a paragraph in the README file.
Thanks.
src/main/java/dasniko/testcontainers/keycloak/ExtendableKeycloakContainer.java
Outdated
Show resolved
Hide resolved
src/main/java/dasniko/testcontainers/keycloak/ExtendableKeycloakContainer.java
Outdated
Show resolved
Hide resolved
src/test/java/dasniko/testcontainers/keycloak/KeycloakContainerTest.java
Outdated
Show resolved
Hide resolved
eb7cce4
to
b849a88
Compare
Thanks, comments addressed and documentation amended. I moved the PS: Greetings from a fellow Darmstaedter :-) |
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.
LGTM, thanks!
Thanks for the approval, I don't have the rights to merge though |
Yes, and that's good as this is my repo and would yield to a security issue if anybody would have merge rights. Running the tests take some time and I don't have always this time to wait, so reviewing and merging are separate tasks at different times. |
I've came across a challenge when using this testcontainer in combination with importing realm configuration: Since the env variable for bootstrapping admin accounts are hardcoded, I cannot automatically import a full Keycloak configuration:
Auto-importing configuration like so will fail, if the realm contains an admin user:
The use case for this is that one might want to export and import a 1:1 export of a running keycloak installation for test reasons.
I've added a flag to disable this behavior, see the test.