-
Notifications
You must be signed in to change notification settings - Fork 305
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
fix(keycloak): add realm imports #565
fix(keycloak): add realm imports #565
Conversation
if you can make it backwards compatible (ie not break tests) then i dont see why we wouldnt merge |
My bad! I have fixed a bug I introduced, that prevented to wait for the first user to be added. Additionally I made the cmd configurable, which I think make sense, depending on the use caes. However, I cannot get the tests running locally, neither on this branch nor on main, so I have a hard time verify whether it works consitently now. |
so it seems straightforward enough, so lgtm! also idk if you accounted for this but the command can be a list or a string. since backwards (or anything beyond basic) compatibility is really not a top concern for community modules, im going to merge this, but feel free to follow up before we do a release. not sure how soon that will be since im pretty backlogged and i dont think we have anything to release that is super critical (happy to be corrected about this). |
Backward compatibility will not be an issue, as there was no way to set the docker command in the previous version - it was automatically overwritten to _DEFAULT_DEV_COMMAND by the start() function. So I guess it's good enough for now. Thanks a lot for the thourough review and time! |
🤖 I have created a release *beep* *boop* --- ## [4.4.1](testcontainers-v4.4.0...testcontainers-v4.4.1) (2024-05-14) ### Bug Fixes * Add memcached container ([#322](#322)) ([690b9b4](690b9b4)) * Add selenium video support [#6](#6) ([#364](#364)) ([3c8006c](3c8006c)) * **core:** add empty _configure to DockerContainer ([#556](#556)) ([08916c8](08916c8)) * **core:** remove version from compose tests ([#571](#571)) ([38946d4](38946d4)) * **keycloak:** add realm imports ([#565](#565)) ([f761b98](f761b98)) * **mysql:** Add seed support in MySQL ([#552](#552)) ([396079a](396079a)) * url quote passwords ([#549](#549)) ([6c5d227](6c5d227)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
With the option to import keycloak realms, introduced with #565, the[_configure()](https://github.com/testcontainers/testcontainers-python/blob/78b6f0ecb15e8cba687eb4588c5ce19ca32208bc/modules/keycloak/testcontainers/keycloak/__init__.py#L57) method is called twice. Once it is called in the [start()](https://github.com/testcontainers/testcontainers-python/blob/78b6f0ecb15e8cba687eb4588c5ce19ca32208bc/modules/keycloak/testcontainers/keycloak/__init__.py#L83) method of keycloak itself and then it is called a second time in the [start()](https://github.com/testcontainers/testcontainers-python/blob/78b6f0ecb15e8cba687eb4588c5ce19ca32208bc/core/testcontainers/core/container.py#L90) method of DockerContainer. This wasn't an issue so far. But if a realm shall be imported (self.has_realm_import in keycloak is True), then every time the string " --import-realm" is added to the start command in the _configure() method. The keycloak container won't start if "--import-realm" is specified multiple times. This is probably the easiest solution to solve the issue. If wished, I can also work on a more robust solution, e.g. by storing the start command in a list and checking that "--import-realm" is only added once. Co-authored-by: Sebastian Scholz <sebastian.scholz@cemocom.de>
This PR adds an option to start a keycloak container with importing one or more realms. This mirrors a feature present in the java keycloak testcontainer.