feat: added support for Podman as container engine#96
Conversation
User Test ResultsTest specification and instructions User tests are not required |
mcdurdin
left a comment
mcdurdin
left a comment
There was a problem hiding this comment.
This looks clean and straightforward, just rename BUILDER_ENGINE to CONTAINER_ENGINE please for clarity
| elif [[ "${CONTAINER_ENGINE}" == "docker" ]]; then | ||
| FILE="-f Dockerfile " | ||
| elif [[ "${CONTAINER_ENGINE}" == "podman" ]]; then | ||
| FILE="-f Podmanfile " |
There was a problem hiding this comment.
Are these really needed? Won't podman use Podmanfile by default just as docker uses Dockerfile by default?
There was a problem hiding this comment.
Podman uses "Dockerfile" for default to be compatible. Usually you can run Podman with the same file as Docker uses but the Dockerfile must address specific requirements of Podman e.g. full path to images.
If you like you can test the build on Docker with the Podmanfile, it should work.
There was a problem hiding this comment.
So if we can get to the point where Podman and Docker can use the same file, that will definitely be better for maintenance. The differences seem pretty minimal at present, so can we get to a point where we can merge them?
There was a problem hiding this comment.
Yes, absolutely. But even if Docker works perfectly with the slightly modified version, I probably would stick to the current solution of the script. Just to have the ability to switch the files. For all projects where we are sure that we can use the same file, I would could create a symbolic link instead.
In case of the Dockerfile for help.keyman.com I think the switch user statement in ln. 18 is an error and will break the build in Docker, too. (Take note of the install command afterwards that will not run with the user "www-data".) But I didn't want to change anything on the existing files without the ability to test it.
|
Test-bot: skip |
3 participants
No description provided.