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

fix: allow windows mounts to reference docker volumes #274

Merged
merged 1 commit into from
Jun 18, 2021

Conversation

RichieSams
Copy link
Contributor

@RichieSams RichieSams commented Jun 17, 2021

MountParser assumes that it's parsing a local absolute path for the src arg. However, it's also legal to use a named docker volume. This change allows for that case.

  • [ x ] I have read the contributing guidelines
    and signed the CLA.
  • [ x ] I have read the security policy.
  • [ x ] I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got green light (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • [ x ] I have added tests that prove my fix is effective or that my feature
    works.
  • [ x ] I have added necessary documentation within the code base (if
    appropriate).

@aeneasr
Copy link
Member

aeneasr commented Jun 17, 2021

Does that keep backwards compatibility? If so, could you reference this in some docs from docker? Thank you! :)

@RichieSams
Copy link
Contributor Author

RichieSams commented Jun 17, 2021

https://docs.docker.com/storage/bind-mounts/#choose-the--v-or---mount-flag
https://docs.docker.com/storage/volumes/#choose-the--v-or---mount-flag

From what I can tell from those docs, even what is put for non windows is incorrect.
The format is:

<src>:<dest>[:<options>]

options is optional.
For volume mounting, src is the name of a volume
For bind-mounts, src is the absolute path on the host

For bind-mounts on Windows, since src is required to be an absolute path, src will have an extra colon, due to the "drive".

I will update the PR accordingly

@RichieSams
Copy link
Contributor Author

I found it difficult to try to re-capture all of the edge cases. So instead, I'm importing / using the same function used by docker-cli to parse the volume string.

volume mount paths should be of the form:
    `<src>:<dest>[:<options>]`

Docs:
https://docs.docker.com/storage/bind-mounts/#choose-the--v-or---mount-flag
https://docs.docker.com/storage/volumes/#choose-the--v-or---mount-flag

`src` can either be the name of a docker volume OR an absolute path on the host

MountParser assumes that it's parsing a local absolute
path for the `src` arg. However, as mentioned above, 
it's also legal to use a named docker volume. In addition, 
there can be the optional `options` section, with an
additional colon.

Rather than trying to capture all the edge cases coding
it ourselves, we just import the same function that
docker-cli uses to parse the volume.
@aeneasr
Copy link
Member

aeneasr commented Jun 18, 2021

That makes a ton of sense, thank you!

This pull request was closed.
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.

2 participants