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: relative parent paths on bind mount src #4966

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Benehiko
Copy link
Member

This is a follow-up PR of #3469

- What I did
The source value of -v/--volume/--mount is validated, if not absolute and contains the . prefix the value is converted to an absolute path. This prefix (.) is always required for source since the -v/--volume/--mount flags also support setting named volumes.

- How I did it

- How to verify it

  • CI tests
  • docker run --rm -v ../:/test busybox ls -l /test
  • docker run --rm --mount type=bind,source=../,target=/test busybox ls -l /test

- Description for the changelog

The run command accepts relative parent paths (../) on -v/--volume and -m/--mount.

- A picture of a cute animal (not mandatory but encouraged)
image

Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
@codecov-commenter
Copy link

Codecov Report

Merging #4966 (3342b07) into master (b39bbb4) will increase coverage by 0.00%.
The diff coverage is 50.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4966   +/-   ##
=======================================
  Coverage   61.18%   61.19%           
=======================================
  Files         294      294           
  Lines       20538    20538           
=======================================
+ Hits        12566    12568    +2     
+ Misses       7077     7076    -1     
+ Partials      895      894    -1     

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving a "request changes" as this may need discussion / consensus; the --mount "advanced" syntax has various parts that were intentionally lower level (and not applying magic, but instead "as provided").

Maybe it's ok to make this change (as it still requires "opt-in" through ./ as prefix), but it's good to double check if we don't overlook situations where this could be problematic.

(If not, then happy to remove my "request for changes")

@thaJeztah
Copy link
Member

Oh, wait; guess I'm reading wrong; this is adding ../ in addition to ./ for both? (and we already had ./ for the --mount syntax?

@Benehiko
Copy link
Member Author

Oh, wait; guess I'm reading wrong; this is adding ../ in addition to ./ for both? (and we already had ./ for the --mount syntax?

Yes! :)

@thaJeztah
Copy link
Member

Gotcha! I'm slightly on the fence if this would be fixing a problem, or also potentially result in confusing / ambiguous situations;

docker run -it --rm alpine
/ # ls .
bin    dev    etc    home   lib    media  mnt    opt    proc   root   run    sbin   srv    sys    tmp    usr    var
/ # ls ..
bin    dev    etc    home   lib    media  mnt    opt    proc   root   run    sbin   srv    sys    tmp    usr    var
/ # ls ../../../../
bin    dev    etc    home   lib    media  mnt    opt    proc   root   run    sbin   srv    sys    tmp    usr    var

FWIW; the current directory case on its own already had some consequences (i.e., the path you specify will be used on thee daemon side to mount, where the daemon could be "a remote machine").

Before that patch, we didn't decide to add local resolving of the path, as technically it would be "incorrect" (or at least add to the expectation that bind-mounts happen from the client side); we ultimately chose to implement it as mounting the "current" directory was a very common use-case, and "worked" on either Docker Desktop or a local daemon.

@Benehiko
Copy link
Member Author

I don't think this is fixing anything, more of a nice to have IMO. I'm sure there are situations where you'd like to specify a parent directory to bind the volume to while running from the current directory.

docker run --rm --mount type=bind,source=../../my-host/parent/directory,target=/test busybox ls -l /test

@Benehiko
Copy link
Member Author

It seems based on the original discussion we allowed to have a relative path as long as it started with the . syntax.

From the above, I (personally) think it'd be ok to allow relative paths; with the constraints that relative paths MUST start with a period (.) and either followed by a path separator (/ or .\), or a colon (for the -v shorthand syntax); that way, relative paths can be used both for the --mount and for the -v / --volume short-hand syntax (without the ambiguity between some-dir being a volume name or a directory named some-dir).

#1203 (comment)

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.

None yet

3 participants