Would it perhaps make sense to use an interface similar to how we currently handle environment variables, e.g. pub fn volume(&mut self, source: impl AsRef<Path>, destination: impl AsRef<Path>) -> &mut Self?

See this PR for a full implementation using that approach.

Would it perhaps make sense to use an interface similar…

docker run supports multiple volume args: --volume /a:/a --volume /b:/b.

This changeset supports specifying multiple --volume from that vector of strings:

ContainerConfig::new().volumes(["/shared/cache:/workspace/cache", "/shared/models:/workspace/models"])

…would generate the command:

docker run --volume /shared/cache:/workspace/cache --volume /shared/models:/workspace/models

Passing these docker run arguments through directly seems cleanest to me. I'm not a fan of creating new interface where they're not required. The whole purpose of this is to build a Docker command, so I do not see the advantage of a more complex implementation here.

@runesoerensen if y'all would prefer that more granular interface implementation, that's your call. We can close this PR and use yours instead.

I just hope to have this implemented/merged, so that I can switch my dependent project back to using the main distribution of libcnb.rs, instead of my branch docker-volume.

docker run supports multiple volume args: --volume /a:/a --volume /b:/b.

This changeset supports specifying multiple --volume from that vector of strings:

The implementation I proposed actually also supports specifying multiple volume/bind mounts (and arguably in a way that's more consistent with the way the parameters are passed to the docker CLI). I've since changed the semantics a bit to use bind_mount rather than volume, but you can still mount multiple paths by chaining bind_mount calls on the container configuration builder:

ContainerConfig::new()
    .bind_mount(
        PathBuf::from("/shared/cache"),
        PathBuf::from("/workspace/cache"),
    )
    .bind_mount("/shared/models", "/workspace/models"); // Less verbose, but retaining the `PathBuf` semantics.

My primary motivation for suggesting this approach is that it's more consistent with how ContainerConfig is currently used to construct Docker CLI arguments. For instance, when setting environment variables, the string manipulation required to specify --env KEY=VALUE is handled here, which joins the KEY and VALUE with =.

On that note, calling the ContainerConfig's volumes function as implemented in this PR will overwrite the volumes field. It seems this breaks a bit with the builder pattern, and may at least be less intuitive in this context (for instance, the similar envs function inserts items into the existing collection, allowing for multiple chained calls to add more envs without dropping the previous state).

Not looking to over-engineer things here, but as mentioned above I changed the implementation from using the --volume parameter to --mount, which seems more accurate and allow for better semantics. Docker's --volume parameter is a "legacy" parameter and not recommended for new implementations, combining multiple different operations (e.g. creating the source/host directory if it doesn't exist (unlike --mount type=bind,source={},target={}, which will return an error if the host directory doesn't exist), allowing for both (named and anonymous) volume and bind mounts), in ways that aren't immediately obvious. So rather than having a volumes field on ContainerConfiguration there's now a bind_mounts field. This allows us to have a cleaner interface for the bind_mount function, which takes source and target impl Into<PathBuf> parameters. If we want to support volume mounts in the future, we could for instance add a volume_mount function taking a String source (i.e. the name of the volume) and an impl Into<PathBuf> target.

Definitely open to learn how my proposed approach here may be flawed (I'm still a bit of a Rust rookie after all :)). And I hear you on wanting to get some solution implemented -- will ask the team to take a look and chime in on the best way forward!

Thanks @runesoerensen for the deeper explanation, evidence, and alternative PR. I am totally fine with switching from volumes to bind_mounts. That sounds good 👍

Ok great, I've merged the PR and cut a new release, so you can switch back to using the version published on crates.io.

I'll close this PR as the functionality is now supported - thanks for your work on this @mars !