-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Support parsing of named pipes for compose volumes #560
Conversation
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Codecov Report
@@ 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 |
There was a problem hiding this 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, `\\`) { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
It's so convenient to pick the Docker CLI from CircleCI build. So I gave it a try
And with backslashes
running against a Windows Insider with Docker engine 17.09.0-rc2 installed. Is this another check in cli or in engine? |
Yes, as I mentioned in moby/moby#34795 (comment), this only fixes the client side. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🐸
Related to moby/moby#34795
Fixes the client side parsing of volume specs to support windows named pipes.