Some fixes for Docker library #6
Conversation
* Use frozen_string_literal magic comment instead of calling #freeze on each string. This has been supported since Ruby 2.3. * Call this 0.3.0.a1. * Update HOMEPAGE with new repository URL.
|
TODO:
|
This solves the issue of running Selenium tests on Arm Mac (via Podman Desktop), among other issues.
|
I've now added tests. Thinking on it, it's not appropriate to merge the Kubernetes change into this one. So I believe this is ready. |
| end | ||
|
|
||
| def env_is_podmanish? | ||
| ENV.fetch('container', '') == 'podman' |
There was a problem hiding this comment.
I might be overthinking this, but from my reading of the podman-run documentation the contract is for the presence of a file rather than a particular environment variable:
Several files will be automatically created within the container… Additionally, a container environment file is created in each container to indicate to programs they are running in a container. This file is located at /run/.containerenv (or /var/run/.containerenv for FreeBSD containers).
I did see a few StackOverflow/similar comments referencing ENV.container, however the above was the only official-ish thing I could find. So I think what you have here works incidentally, but maybe isn't the actual contract. What do you think?
There was a problem hiding this comment.
The contract is there for systemd (cite comment from podman/podman#11836), but yes, it is probably safer to use the file specified in podman-run.1. Would you rather this be added as another File.exist? or keep it in a separate private method?
There was a problem hiding this comment.
Thanks for the systemd link. That actually is a bit more general:
To allow systemd (and other programs) to identify that it is executed within a container, please set the $container environment variable for PID 1 in the container to a short lowercase string identifying your implementation. With this in place the ConditionVirtualization= setting in unit files will work properly. Example: container=lxc-libvirt
I.e. it just has to be set, and doesn't necessarily have to be "podman". (Even for podman itself; there are some comments saying it's set differently in certain versions.)
So, I would probably make this one a bit more general (referencing the systemd spec), and if we really want to tighten up the podman check we could have a separate method checking for /run/.containerenv or /var/run/.containerenv. How does that sound?
There was a problem hiding this comment.
I think that approach makes more sense in general, but will drastically complicate the test code as it stands now.
I'm thinking we might be able to clean it up by testing each function separately (init_cgroup_is_dockerish?, has_podman_file?, has_container_env?) and then testing the running_in_docker? by simply mocking each one to return true/false, as in:
allow(Docker).to receive_messages(init_cgroup_is_dockerish?: true, has_podman_file?: false)Though, that is still a lot of combinations, and will rapidly become even more unwieldy as we add more (i.e. #5).
frozen_string_literaldeclaration at the top of ModuleInfo.#running_in_container?.The Podman check allows Selenium tests to run and pass from within Podman Desktop on my Arm Mac, and should also work for Linux systems using Podman Desktop. This also future-proofs us in case we decide to migrate from Docker to Podman at a later date.