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

Support parsing of named pipes for compose volumes #560

Merged

Conversation

dnephin
Copy link
Contributor

@dnephin dnephin commented Sep 26, 2017

Related to moby/moby#34795

Fixes the client side parsing of volume specs to support windows named pipes.

Signed-off-by: Daniel Nephin <dnephin@docker.com>
@codecov-io
Copy link

codecov-io commented Sep 26, 2017

Codecov Report

Merging #560 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #560      +/-   ##
==========================================
+ Coverage    49.5%   49.51%   +<.01%     
==========================================
  Files         208      208              
  Lines       17153    17156       +3     
==========================================
+ Hits         8491     8494       +3     
  Misses       8230     8230              
  Partials      432      432

Copy link
Member

@StefanScherer StefanScherer left a comment

Choose a reason for hiding this comment

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

Thanks for starting this PR.
To have Docker as a tool that gets out of the users way please support (unix) slashes as well. ❤️ 🙏 😃

@@ -112,6 +112,11 @@ func isFilePath(source string) bool {
return true
}

// windows named pipes
if strings.HasPrefix(source, `\\`) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can make this slash-insensitive (like case-insensitive, but for slashes ;-) ).
From a Mac/Linux client remote controlling a Windows swarm node I guess many people still are used to unix slashes.

I also use eg. docker run -v //./pipe/docker_engine://./pipe/docker_engine -it -p 8080:8080 stefanscherer/visualizer-windows:insider right now and it works pretty good as Golang itself can handle backslashes and slashes in paths.

Slashes because bash needs extra escaped backslashes as you can see here.

´´´
$ docker run -v \.\pipe\docker_engine:\.\pipe\docker_engine -it -p 8080:8080 stefanscherer/visualizer-windows:insider
docker: Error response from daemon: invalid bind mount spec "\.pipedocker_engine:\.pipedocker_engine": invalid volume specification: '.pipedocker_engine:.pipedocker_engine'.



(oops as you can see even GitHub looses some backslashes although I'm using three backticks here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normal slash is already handled in this function on the line above (111). A single / already returns true.

Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@StefanScherer
Copy link
Member

StefanScherer commented Sep 26, 2017

It's so convenient to pick the Docker CLI from CircleCI build. So I gave it a try

$ wget https://5607-88013053-gh.circle-artifacts.com/0/work/build/docker-darwin-amd64
$ chmod +x docker-darwin-amd64
$ ./docker-darwin-amd64 service create --name whoami --mount type=bind,source=//./pipe/docker_engine,destination=//./pipe/docker_engine stefanscherer/whoami:insider
vkwsqpgcgm3nso31nw5no8fcx
overall progress: 0 out of 1 tasks 
1/1: invalid mount target, must be an absolute path: //./pipe/docker_engine 

And with backslashes

$ ./docker-darwin-amd64 service create --name whoami --mount 'type=bind,source=\\.\pipe\docker_engine,destination=\\.\pipe\docker_engine' stefanscherer/whoami:insider
cruvatiet5yhuxta2teppqw8b
overall progress: 0 out of 1 tasks 
1/1: invalid mount target, must be an absolute path: \\.\pipe\docker_engine 

running against a Windows Insider with Docker engine 17.09.0-rc2 installed.

Is this another check in cli or in engine?

@dnephin
Copy link
Contributor Author

dnephin commented Sep 26, 2017

Yes, as I mentioned in moby/moby#34795 (comment), this only fixes the client side. docker service create does not do any parsing on the client side. This still needs to be fixed in moby/moby (daemon/cluster/executor/container/validate.go).

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.

LGTM

Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐸

@vdemeester vdemeester merged commit 448d56a into docker:master Oct 3, 2017
@GordonTheTurtle GordonTheTurtle added this to the 17.11.0 milestone Oct 3, 2017
@dnephin dnephin deleted the fix-bind-mount-named-pipe-compose branch October 11, 2017 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants