-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
Set ping version even on error #33827
Conversation
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.
changes LGTM, but left a suggestion for adding a bit more context to the tests
client/ping_test.go
Outdated
assert.Equal(t, "awesome", ping2.APIVersion) | ||
} | ||
|
||
func TestPingWithError(t *testing.T) { |
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.
Isn't this test exactly the same as TestPingFail()
(with withHeader = true
)?
oh, only the error returned not being nil
Perhaps it's worth adding a short description to each of the tests, describing what exactly we're testing for. We recently found some tests that (after having been refactored over the years) were not testing anything, but nobody noticed, because there was no clear understanding what was originally being tested for.
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.
Sure, updated with doc strings. These tests are mostly exercising various code paths to ensure there's no panics.
799a493
to
2709077
Compare
Related to docker/cli#149 |
client/ping.go
Outdated
return ping, err | ||
} | ||
defer ensureReaderClosed(serverResp) | ||
if serverResp.header != nil { |
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.
cli.doRequest
does two things - first it issues the actual HTTP request with ctxhttp.Do
, then it checks the status code to determine whether the request was successful.
Could we separate those two things, so we can distinguish a HTTP-level problem from an application-level problem, and only check the headers when the HTTP request went through? That would avoid using serverResp
even when an error is returned, which can be a problematic pattern.
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.
So, I agree a fair amount of the handling in doRequest
doesn't even make sense in the client code and should be handled by the CLI instead.
However I think this nil check would still be required. I'm also not sure it's right to refactor the whole client lib in order to fix this issue with ping details.
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 was thinking we could split the if serverResp.statusCode < 200 || serverResp.statusCode >= 400 {
block into a separate function that gets called after doRequest
, if doRequest
succeeds. The API-Version
and Docker-Experimental
header handling code could go in between. This wouldn't constitute a major refactoring, and it means the caller could immediately return an error if doRequest
returns an error.
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.
PTAL
2709077
to
b7412f4
Compare
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 👍
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, but left a suggestion - let me know if you want to address that in this PR or as a follow up
client/ping.go
Outdated
} | ||
|
||
ping.OSType = serverResp.header.Get("OSType") | ||
if err := cli.checkResponseErr(serverResp); err != nil { |
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.
nit: this if isn't needed, and could be just;
(also not sure if the defer
was needed?)
err := cli.checkResponseErr(serverResp)
ensureReaderClosed(serverResp)
return ping, err
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.
Sure updated.
In some cases a server may return an error on the ping response but still provide version details. The client should use these values when available. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
b7412f4
to
27ef09a
Compare
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, thanks!
Includes changes from; - Fix handling of remote "git@" notation (moby/moby#33696) - Move some `api` package functions away moby/moby#33798 (related to docker#236) - Update go-connections dependency moby/moby#33814 (already vendored in docker#238) - Set ping version even on error (moby/moby#33827) - Do not add duplicate platform information to service spec (moby/moby#33867) - Refactor MountPoint Setup function in volume.go (moby/moby#33890) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Includes changes from; - Add a LastTagTime for images (moby/moby#31497) - Fix handling of remote "git@" notation (moby/moby#33696) - Move some `api` package functions away (moby/moby#33798) (related to docker#236) - Set ping version even on error (moby/moby#33827) - Do not add duplicate platform information to service spec (moby/moby#33867) - Refactor MountPoint Setup function in volume.go (moby/moby#33890) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Includes changes from; - Add a LastTagTime for images (moby/moby#31497) - Fix handling of remote "git@" notation (moby/moby#33696) - Move some `api` package functions away (moby/moby#33798) (related to docker#236) - Set ping version even on error (moby/moby#33827) - Do not add duplicate platform information to service spec (moby/moby#33867) - Refactor MountPoint Setup function in volume.go (moby/moby#33890) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Includes changes from; - Add a LastTagTime for images (moby/moby#31497) - Fix handling of remote "git@" notation (moby/moby#33696) - Move some `api` package functions away (moby/moby#33798) (related to docker#236) - Set ping version even on error (moby/moby#33827) - Do not add duplicate platform information to service spec (moby/moby#33867) - Refactor MountPoint Setup function in volume.go (moby/moby#33890) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Includes changes from; - Add a LastTagTime for images (moby/moby#31497) - Fix handling of remote "git@" notation (moby/moby#33696) - Move some `api` package functions away (moby/moby#33798) (related to docker/cli#236) - Set ping version even on error (moby/moby#33827) - Do not add duplicate platform information to service spec (moby/moby#33867) - Refactor MountPoint Setup function in volume.go (moby/moby#33890) Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: 366d3ec971d8007c667e8d7dc8e35a346fb19539 Component: cli
Includes changes from; - Add a LastTagTime for images (moby/moby#31497) - Fix handling of remote "git@" notation (moby/moby#33696) - Move some `api` package functions away (moby/moby#33798) (related to docker#236) - Set ping version even on error (moby/moby#33827) - Do not add duplicate platform information to service spec (moby/moby#33867) - Refactor MountPoint Setup function in volume.go (moby/moby#33890) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
In some cases a server may return an error on the ping response but
still provide version details. The client should use these values when
available.