Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: directory mounting supports bind and volume modes #122

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

DDSDerek
Copy link

fix #121

@lavie

@lavie
Copy link
Owner

lavie commented Dec 29, 2024

Thanks.

Why must we add :rw when it's the default?
https://docs.docker.com/engine/storage/volumes/#options-for---volume

Similarly, we add a : prefix when the volume name is omitted, which means we can no longer generate a clean-looking 'run --volume some_path`, which is what the test fixture actually uses.

I prefer to default to the cleanest syntax, even though it's less explicit, because otherwise even a seemingly simple container ends up having tons of options in its run command just because they're all default values.

@DDSDerek
Copy link
Author

DDSDerek commented Dec 30, 2024

Thanks.

Why must we add :rw when it's the default? https://docs.docker.com/engine/storage/volumes/#options-for---volume

Similarly, we add a : prefix when the volume name is omitted, which means we can no longer generate a clean-looking 'run --volume some_path`, which is what the test fixture actually uses.

I prefer to default to the cleanest syntax, even though it's less explicit, because otherwise even a seemingly simple container ends up having tons of options in its run command just because they're all default values.

Thanks for pointing that out, I've made the changes in 6c2a4b1.
Only add :ro if the user specifies read-only mount.
Don't add it for the default case.
And added a test unit to deal with the situation where the user specifies to use read-only mode.

@lavie
Copy link
Owner

lavie commented Dec 30, 2024

Appreciated! Could I also ask you to please omit the source/name and the : completely in case they are empty/unspecified?
So we could have run -v /mount instead of run -v :/mount

@DDSDerek
Copy link
Author

Appreciated! Could I also ask you to please omit the source/name and the : completely in case they are empty/unspecified? So we could have run -v /mount instead of run -v :/mount

For this situation, there is a solution in #115 (comment)

@DDSDerek
Copy link
Author

Appreciated! Could I also ask you to please omit the source/name and the : completely in case they are empty/unspecified? So we could have run -v /mount instead of run -v :/mount

I added the -use-volume-id parameter to let users choose whether to use the volume automatically allocated by Docker.
By default, it is not used, which is consistent with the current state.

@DDSDerek
Copy link
Author

I hope you can review it again and point out if there are any problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants