integration-cli: Generate unix-style volume paths for tests #10794
Conversation
|
| t.Fatal(err) | ||
| } | ||
|
|
||
| bindPath := path.Join("/tmp", fmt.Sprintf("test.%s", makeRandomString(10))) |
There was a problem hiding this comment.
Shouldn't this be filepath, not path?
Nevermind, this is on Windows, so yeah, path is what we want :)
There was a problem hiding this comment.
^^ so, this might confuse people, because they think it should be replaced by filepath (because that's usually the right way); I think a comment should be added to these lines to explain why this is used and to prevent people from refactoring them by accident.
There was a problem hiding this comment.
@cpuguy83 why would filepath be wrong?
There was a problem hiding this comment.
If I understood this PR correctly: because of #10794 (comment); filepath would attempt to create a Windows-style path (C:\temp) on the daemon, but the daemon is running on linux
There was a problem hiding this comment.
Yep. Generating the path on Windows, but the path is for Linux.
There was a problem hiding this comment.
one thing that I've been wondering about... in all of my windows programming I've always been able to use / instead of \ in my paths - is this not true for golang? Just wondering if we really need to use filepath.Join() when we're on windows at all.
There was a problem hiding this comment.
I like the idea for some kind of utility to prevent people tripping over this (something like daemon.filepath()?) then no comments would be required, and it would keep working if eventually a Windows Daemon arrives (just thinking out loud here :))
There was a problem hiding this comment.
@duglin normally it might work a little bit, but you're passing c:\users\xxx\appdata\temp\test.123 to docker daemon. no way that's working with colon symbol and all :)
|
So, we need paths a lot in the tests. Maybe we should make a helper for creating temp paths like this since we can't use the normal mechanisms. |
|
@cpuguy83 extracted into a helper. |
Some tests in `docker_api_containers_test.go` assume the docker daemon is running at the same machine as the cli and uses `ioutil.TempDir` to create temp dirs and use them in the test. On windows ioutil.TempDir and os.TempDir would create win-style paths and pass them to daemon. Instead, I hardcoded `/tmp/` and generate some random path manually and allow daemon to create the directory. Fixes tests: - TestContainerApiStartVolumeBinds - TestContainerApiStartDupVolumeBinds - TestVolumesFromHasPriority Downside: - Does not clean the temp dirs generated on the remote daemon machine unless delete container deletes them. Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>
|
Sweet, LGTM |
|
kewl LGTM |
…i-volume-path-fix integration-cli: Generate unix-style volume paths for tests
Some tests in
docker_api_containers_test.goassume thedocker daemon is running at the same machine as the cli
and uses
ioutil.TempDirto create temp dirs and use themin the test.
On windows ioutil.TempDir and os.TempDir would create win-style
paths and pass them to daemon. Instead, I hardcoded
/tmp/test.{{randString}}and test generates some random path manually and allow daemon to create
the directory on daemon-side.
Fixes tests:
Downside:
Signed-off-by: Ahmet Alp Balkan ahmetalpbalkan@gmail.com
cc: @unclejack @cpuguy83 @estesp @jfrazelle
Label:
#windows