Repository navigation
Conversation
…g based on selected engine and replace as uses of hard-coded `"docker"` with this value
…ts official API client
|
Also added untested integration test variant that uses Podman and the Podman engine support I tested this in NixOS and it works without the argument patching script at #346 (comment). |
stepchowfun
left a comment
stepchowfun
left a comment
There was a problem hiding this comment.
Thank you for this PR! Left some comments.
| # This will be called when the task finishes, regardless of whether it succeeded. | ||
| function cleanup { | ||
| # Delete the container created below to run the integration tests. | ||
| docker container rm --force quay.io/podman/stable |
There was a problem hiding this comment.
I think the container name here has to match the name given below (--name podman).
| artifacts/docuum-aarch64-unknown-linux-musl | ||
|
|
||
| validate_release: | ||
| validate_release_docker: |
There was a problem hiding this comment.
I see the CI still tries to run validate_release and fails due to this rename. One solution is to make a new validate_release task that depends on validate_release_docker and validate_release_podman:
validate_release:
description: Validate the release.
dependencies:
- validate_release_docker
- validate_release_podman| quay.io/podman/stable | ||
| docker container cp "artifacts/docuum-x86_64-unknown-linux-musl" podman:/ | ||
| docker container cp integration-test.sh podman:/ | ||
| docker container exec podman ./integration-test.sh podman |
There was a problem hiding this comment.
When I run this, the test doesn't seem to pass:
+ docker container run --privileged --name podman --detach quay.io/podman/stable
bf6f56a3de472531bb18516e9944120d57fe58e862efc6be87568827fa4cea99
+ docker container cp artifacts/docuum-x86_64-unknown-linux-musl podman:/
+ docker container cp integration-test.sh podman:/
+ docker container exec podman ./integration-test.sh podman
Error response from daemon: container bf6f56a3de472531bb18516e9944120d57fe58e862efc6be87568827fa4cea99 is not running
Does it work for you?
|
|
||
| - Omit the `--tty` flag. This prevents Docuum from printing colored logs, which you probably don't want for a daemon. | ||
| - Configure Docker as a hard dependency. Ordinarily, Docuum and Docker can be started in any order, and Docuum will patiently wait for Docker to start if needed. However, when running Docuum as a Docker container, then of course Docker must be started first. | ||
| - If using Docker, configure Docker as a hard dependency. Ordinarily, Docuum and Docker can be started in any order, and Docuum will patiently wait for Docker to start if needed. However, when running Docuum as a Docker container, then of course Docker must be started first. |
There was a problem hiding this comment.
Since this is inside a section called "Running Docuum in a Docker container on a host capable of running Linux containers", I think maybe "If using Docker" reads a bit odd. Maybe we can adapt this whole section to apply to both Docker and Podman? (I see some change in that direction above.)
There was a problem hiding this comment.
I left the wording of “Docker container” as-is as I see it as a synonym for “OCI container”. I can change the words “Docker container” to something else but otherwise I think the section is already made generic over Docker vs Podman.
(Minus the mistakes in that section you also noticed, of course.)
| } | ||
| trap cleanup EXIT | ||
|
|
||
| # Run the integration test suite [tag:integration_test_step]. Uses Podman-in-Docker. |
There was a problem hiding this comment.
Tagref will complain about the duplicate [tag:integration_test_step] tag. I think we can fix it by changing it to [group:integration_test_step] (and leave corresponding the ref in integration-test.sh unchanged).
|
|
||
| // Ask Docker for the ID of an image. | ||
| fn image_id(image: &str) -> io::Result<String> { | ||
| fn image_id(command: &str, image: &str) -> io::Result<String> { |
There was a problem hiding this comment.
I think this would be improved by passing an Engine around instead of a command string, and then we could have a helper function that produces then name of the binary from the engine. This is not really the "command" but the name of the binary (the command would include the args).
There was a problem hiding this comment.
Oh I see, the user can customize the name/path of the binary in addition to setting the engine. Maybe being able to configure the engine is enough.
There was a problem hiding this comment.
On NixOS, the engine binary is not in $PATH by default. It can be explicitly added there of course but I actually implemented this whole thing by adding binary name passing first and then using the engine only for setting the default binary name.
| fn str_to_oscow(s: &str) -> Cow<'_, OsStr> { | ||
| <str as AsRef<OsStr>>::as_ref(s).into() | ||
| } |
There was a problem hiding this comment.
Maybe we can just build a Vec<String> (as before)?
2 participants
"docker"everywherepodman-remote(symlinked as justpodman)Status: Ready
Fixes: #346