-
-
Notifications
You must be signed in to change notification settings - Fork 191
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
Refactoring code for podman support #234
base: dev
Are you sure you want to change the base?
Conversation
Hello Thanks for this PR, it is indeed a more complete approach to integrate podman in a stable way in Exegol.
Thanks in any case for your help with this integration, there will be a lot of testing to do to make sure everything is compatible with podman and exegol before officially integrating it into the project, but it's certainly a very good starting point. |
I'll try to give answer
FIXED on podman-py upstream. I wouldn't suggest to add
FIXED. PS: in the future
FIXED. Meanwhile I try to align it with
Not all functions are the same. The risk of divergence depends on how we will adapt podman on Exegol. On this PR we are not using Docker Py functions to manage podman daemon, but we are invoking original Podman Py libs to manage its daemon. Overall, before to continue to work on this PR, I think we need to wait for those issues/PRs I opened on Podman Py project. |
b4f83e7
to
008d6fc
Compare
c3b6486
to
e331d70
Compare
To summarize. My PRs on podman python repository have been merged. There are only two issues to manage but they affect directly Podman:
|
Description
Refactoring code to support Podman
Point of attention
TO DO
DockerUtils.py:
Mounts
attribute values. It causes Exegol to raise the errorCurrently the only issue on progress bar is that Podman API does not provide
Extracting
output during pull.podman.from_env()
seems to return exit status0
even when podman service is down or uninstalled. It causes to produce AssertionError on Exegol when podman is down instead of running the right exception. Reported in a comment here: fix: Enhance environment connection error handling containers/podman-py#472 . Currently, it can be managed by usingclient.ping()
.ConstantConfig.IMAGE_NAME in [repo_tag.split(':')[0] for repo_tag in img.attrs.get("RepoTags", [])]:
does not manage well cases where user pulled manually exegol image containing also the registry name, for exampledocker.io/nwodtuhs/exegol
. In this scenario, it won't enter in if statement. It is needed to addany
to manage cases where locally I have more container images. [Fixed in PR]ExegolImage.py:
[FIXED]
As above, cannot enter in if statement in case user pulled manually an image by specifying
docker.io
. Need to usein
instead ofstartswith
. [Fixed in PR]Evaluate if it is needed to manage
if
statements involvingConstantConfig.IMAGE_NAME
in other files to manage the presence of prefixdocker.io
in the image name.[FIXED] ExegolController.py - Import
podman
and manageconsole.print_exception(show_locals=True, suppress=[docker, requests, git])
to consider podman module too.ConstantConfig.py:
Implement the samePodman should already manage mount. Just open this PR on podman py upstream Support uppercase mount attributes containers/podman-py#487from docker.types import Mount
for podman: [FEATURE] - Implement Mount type containers/podman-py#458