fix(up): forward single-file bind mounts to container run#94
Conversation
Apple `container` (0.12.3+) accepts `--volume host/file:container/file[:mode]` for single-file bind mounts, but `configVolume` was branching on `isDirectory` and silently dropping the mount (with only a warning) when the host side resolved to a file. Common compose idioms like `./config.yaml:/app/config.yaml:ro` or `./init.sh:/docker-entrypoint-initdb.d/init.sh` therefore never reached the container. Forward the bind mount for both file and directory sources, and preserve the optional mode component (e.g. `:ro`) when rebuilding the `--volume` argument — previously the mode was stripped even for directory mounts. Closes Mcrich23#93
|
Hi @csaller. Thank you for your PR. I plan to look at it early next week when I have some time (if I don't send something, please ping me!) For the test, go ahead and add it. Using an |
Extract configVolume's bind-mount logic into a public free function composeVolumeToRunArgs so it can be tested without spinning up a real container runtime. Add 7 static tests covering: single-file mount with :ro mode (the bug case), single-file mount without mode, directory mount, directory mount with mode, relative path resolution, missing host path auto-creation, and invalid format.
|
Hi @Mcrich23, thanks! I just added the unit test and they are passing locally, also a test build worked exactly as expected with no regressions detected. Lmk if you need everything else from me. Other than that, have a great weekend! |
The function only needs to be reachable from the test target via @testable import ContainerComposeCore — there's no reason to expose it as part of the public API surface.
|
Hi @Mcrich23! pinging you on this again |
|
Oh my gosh, I totally forgot about this. Thank you. Will review tonight! |
| let fullHostPath = (source.starts(with: "/") || source.starts(with: "~")) ? source : (cwd + "/" + source) | ||
|
|
||
| if fileManager.fileExists(atPath: fullHostPath) { | ||
| args.append("-v") | ||
| args.append(bindMountArg(source: source)) | ||
| } else { | ||
| do { | ||
| try fileManager.createDirectory(atPath: fullHostPath, withIntermediateDirectories: true, attributes: nil) | ||
| print("Info: Created missing host directory for volume: \(fullHostPath)") | ||
| args.append("-v") | ||
| args.append(bindMountArg(source: source)) |
There was a problem hiding this comment.
Even though we are passing it via command and container should handle it just fine, passing an absolute path will help reduce bugs.
|
|
||
| args.append("-v") | ||
| args.append("\(volumePath):\(destinationPath)") | ||
| } |
| private func makeTempDir() throws -> URL { | ||
| let tmp = FileManager.default.temporaryDirectory.appending(path: UUID().uuidString) | ||
| try FileManager.default.createDirectory(at: tmp, withIntermediateDirectories: true) | ||
| return tmp | ||
| } |
There was a problem hiding this comment.
Maybe make a test Trait for this?
Mcrich23
left a comment
There was a problem hiding this comment.
I accidentally approved the PR earlier. If you could please address the issues copilot raised, I would be happy to merge this!
| private func makeTempDir() throws -> URL { | ||
| let tmp = FileManager.default.temporaryDirectory.appending(path: UUID().uuidString) | ||
| try FileManager.default.createDirectory(at: tmp, withIntermediateDirectories: true) | ||
| return tmp | ||
| } |
There was a problem hiding this comment.
Maybe make a test Trait for this?
| let fullHostPath = (source.starts(with: "/") || source.starts(with: "~")) ? source : (cwd + "/" + source) | ||
|
|
||
| if fileManager.fileExists(atPath: fullHostPath) { | ||
| args.append("-v") | ||
| args.append(bindMountArg(source: source)) | ||
| } else { | ||
| do { | ||
| try fileManager.createDirectory(atPath: fullHostPath, withIntermediateDirectories: true, attributes: nil) | ||
| print("Info: Created missing host directory for volume: \(fullHostPath)") | ||
| args.append("-v") | ||
| args.append(bindMountArg(source: source)) |
There was a problem hiding this comment.
Even though we are passing it via command and container should handle it just fine, passing an absolute path will help reduce bugs.
Closes #93.
The bug
configVolumeinComposeUp.swifttreated file vs. directory host sources differently: if the host side of a bind mount resolved to a file, the mount was skipped and only a"The 'container' tool does not support direct file mounts"warning was printed. In practice that warning is often masked by the image-fetch/progress output, so users see a container that silently starts withvolumes:entries missing.But Apple
container(0.12.3+) does support file-to-file bind mounts. This works today, straight from the CLI:So the gate in
container-composeis no longer (and, based on field use, seems to never have been) correct. Common compose idioms like./config.yaml:/app/config.yaml:roor./init.sh:/docker-entrypoint-initdb.d/init.shhave to be worked around by moving the file into a directory that's already mounted.The fix
container --volume.:ro, etc.) when rebuilding the--volumeargument. Previously the mode was stripped even for directory mounts, so:rohad no effect there either.Diff is small and localized to the bind-mount branch of
configVolume. The named-volume branch is untouched.Verification
Minimal reproducer with one directory mount and one file mount:
Before (0.11.0):
container inspectshows only the directory mount; file mount is absent fromconfiguration.mounts;cat /app/hello.txt→No such file or directory.After this patch:
[ { "source": ".../data/", "destination": "/app/data", "options": [], "type": {"virtiofs": {}} }, { "source": ".../hello.txt", "destination": "/app/hello.txt", "options": ["ro"], "type": {"virtiofs": {}} } ]Container logs:
The
:roon the file mount is now propagated (options: ["ro"]), matching compose semantics.Test status
Ran
swift test. The only failures are in the pre-existing dynamic suite ("What goes up must come down - two containers", "Test WordPress with MySQL compose file", "Test compose with complex dependency chain") — all of those use compose YAMLs with only named volumes (wordpress_data,db_data) and hit the known named-volume bootstrap issue tracked in #52. They reproduce onmainwithout this patch; no code path touched by this change is exercised by them.I didn't add a unit test because
configVolumeis a private instance method that pulls from severalComposeUpfields (cwd,fileManager,environmentVariables,projectName). Happy to extract a pure helper and add a static test in a follow-up if you'd prefer that shape.Test plan
docker-compose.ymlwith a single-file bind mount (e.g../config.yaml:/app/config.yaml:ro) brings up a container that can read the file.docker-compose.ymlwith a directory bind mount continues to work.container inspect <name>shows both mounts inconfiguration.mounts, withoptions: ["ro"]preserved for:roentries.