-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
TestGetContainersAttachWebsocket: use DOCKER_TEST_HOST if specified #10747
TestGetContainersAttachWebsocket: use DOCKER_TEST_HOST if specified #10747
Conversation
@@ -292,14 +283,27 @@ func sockRequestRaw(method, endpoint string, data io.Reader, ct string) ([]byte, | |||
var c net.Conn | |||
switch daemonUrl.Scheme { | |||
case "unix": | |||
c, err = net.DialTimeout(daemonUrl.Scheme, daemonUrl.Path, time.Duration(10*time.Second)) | |||
return net.DialTimeout(daemonUrl.Scheme, daemonUrl.Path, time.Duration(10*time.Second)) |
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.
Am I missing something or we should be using the timeout
parameter instead of the time.Duration(10*time.Second)
hardcoded value here? (same issue in the case "tcp":
below)
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.
you're right, fixing now.
TestGetContainersAttachWebsocket is currently broken on Windows CI tests b/c it has hardcoded unix://var/run/docker.sock. This change makes use of @icecrime's code in docker_utils and generalizes it with sockConn() to provide a net.Conn by making use of DOCKER_TEST_HOST. Also fixes the test. Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>
LGTM |
LGTM ❤️ |
(So far in my test run, this is the only test that's failed when I supply a non-default |
Got another offender!
Want to try collecting them all in this one PR, or shall I start opening new PRs for each? 😇 |
@tianon I think there's no need. It'd be just fine if you send PR for utils.fo#daemonHost(). This test was hardcoded to use unix sock. I'm fixing them as I go, no worries. 😄 🌃 😴 |
…ttachWS-fix TestGetContainersAttachWebsocket: use DOCKER_TEST_HOST if specified
TestGetContainersAttachWebsocket is currently broken on Windows CI tests b/c it has hardcoded unix://var/run/docker.sock. (introduced in #10153.) This change makes use of @icecrime's code in docker_utils and generalizes it with sockConn() to provide a net.Conn by making use of DOCKER_TEST_HOST.
Also fixes the test TestGetContainersAttachWebsocket on windows/darwin.
Signed-off-by: Ahmet Alp Balkan ahmetalpbalkan@gmail.com
Label:
#windows
Cc: @acbodine @tiborvass @icecrime @unclejack @jfrazelle