-
Notifications
You must be signed in to change notification settings - Fork 308
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Fix tests on Apple Silicon / M1 #357
Fix tests on Apple Silicon / M1 #357
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.
Thanks for the fixes; left a few inline comments.
Regarding keycloack, there's a possible alternative image discussed in #289, but we don't have a PR yet.
I tried for a few minutes to get the maintained image up and running, but at first sight it looked like it is intended that you specify at least an ENTRYPOINT while using the image as a base image. E.g. it is not 100% Plug'n'Play. I might be wrong though, I did not give myself too much time trying to fix it. I might have some time to look into it at a later stage, but I really want to get .with_reuse() up and running after the Ryuk PR is merged. Might be that it annoys me enough during development that I cannot get all the tests to run on my machine so I fix it then 😅 |
Prevous tests were dependent on a lot of Redis client logic, which is not the goal of this package. This package provides a container with a running Redis version, how you interact with it is up to the user. Changed to simpler test with only basic client usage to make sure container is started and ready to accept connections.
…r logs before starting
…d non-deprecated password env variable
…ime on recent release
super().start() | ||
self._connect() | ||
container = super().start() | ||
wait_for_logs(container, 'Listening on:') |
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 haven't tried this yet, but by passing KC_HEALTH_ENABLED=true
as a environment variable I believe it might be possible to keep the self._connect()
if instead of pooling {url}/auth
we pool {url}/health
.
Hi, just a general comment. Because of the fact that Keycloak changed their architecture (if you look at the tags, there are a bunch of legacy versions) there's no silver bullet to spin up a container and make sure it's up and running. After not a huge time I was able to come up with this solution, it's not perfect but from my testing it seems to be working for all Keycloak versions, including legacy and non legacy ones. class CustomKeycloakContainer(DockerContainer):
def __init__(
self,
image="quay.io/keycloak/keycloak:latest",
username: Optional[str] = None,
password: Optional[str] = None,
port: int = 8080
) -> None:
super(CustomKeycloakContainer, self).__init__(image=image)
self.username = username or os.getenv("KEYCLOAK_USER", "test")
self.password = password or os.getenv("KEYCLOAK_PASSWORD", "test")
self.port = port
self.with_exposed_ports(self.port)
def _get_image_tag(self) -> str:
return self.image.split(":")[1]
def _uses_legacy_architecture(self) -> bool:
tag = self._get_image_tag()
try:
major_version_number = int(tag.split(".")[0])
except ValueError:
major_version_number = 2023
return major_version_number <= 16 or "legacy" in tag.lower()
def _configure(self) -> None:
# There's a bit of confusion as to which version / architecture uses which
# Because of this I'm setting the credentials both ways
self.with_env("KEYCLOAK_ADMIN", self.username)
self.with_env("KEYCLOAK_ADMIN_PASSWORD", self.password)
self.with_env("KEYCLOAK_USER", self.username)
self.with_env("KEYCLOAK_PASSWORD", self.password)
self.with_env("KC_HEALTH_ENABLED", "true")
self.with_env("KC_METRICS_ENABLED", "true")
if not self._uses_legacy_architecture():
self.with_command("start-dev")
def get_url(self) -> str:
host = self.get_container_host_ip()
port = self.get_exposed_port(self.port)
return f"http://{host}:{port}"
def get_base_api_url(self) -> str:
base_url = self.get_url()
return f"{base_url}/auth" if self._uses_legacy_architecture() else base_url
@wait_container_is_ready(requests.exceptions.ConnectionError, requests.exceptions.ReadTimeout)
def _connect(self) -> None:
url = self.get_url()
response = requests.get(f"{url}/auth", timeout=1)
response.raise_for_status()
def start(self) -> "CustomKeycloakContainer":
self._configure()
container = super().start()
if self._uses_legacy_architecture():
self._connect()
else:
wait_for_logs(container, 'Listening on:')
return self This might need some refinement but overall I'm happy with the solution. This is something hard to tackle because of all the changes between versions and architectures and the lack of decent documentation. EDIT: Opened a PR just for this change, you can find it here |
I think we should have a larger discussion in this repo on what we actually want to support in custom container setups, as making the implementation support all versions since the beginning of time will lead to a lot of nasty workarounds while only catering to a (presumably) small portion of people that want to use legacy versions. We need to ask ourselves the following question, and probably a lot more:
Having support for an explicit version range would also require us to cap the support on the latest release, and check / update the version range on every new release (otherwise we cannot know if the new versions are actually compatible or not, and it would be worse to say incorrectly that we support something than not supporting it). I'm making an issue where we can follow this discussion more closely. |
Any change we can get this PR merged? Tests are currently failing on M1/M2 Macs with ARM64 CPUs. |
Closing due to fixes already implemented in other branches |
Updates some tests to make them compatible with M1 / Apple Silicon Macs. Also adds some
wait_for_logs
utilities in tests to avoid race conditions on images with slow startup time (Might be more race conditions in other tests, but these were the ones i encountered myself).Quick summary of changes:
:latest
in tests, for more reproducible builds. Also waits for logs before connecting.Only remaining failing test on Apple Silicon is the
jboss/keycloak
image, which seems to be unmaintained (latest release 1 year ago+ and the images have officially moved toquay.io
. Should we remove this altogether as part of the 4.0 release oftestcontainers
?Update: Updated
keycloak
image to new, maintained repo onquay.io
. Works great on Apple Silicon and other architectures.