Set ping version even on error#33827
Conversation
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
changes LGTM, but left a suggestion for adding a bit more context to the tests
There was a problem hiding this comment.
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.
Sure, updated with doc strings. These tests are mostly exercising various code paths to ensure there's no panics.
cpuguy83
force-pushed
the
return_ping_data_if_available
branch
from
799a493 to
2709077
Compare
|
Related to docker/cli#149 |
There was a problem hiding this comment.
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.
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.
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.
cpuguy83
force-pushed
the
return_ping_data_if_available
branch
from
2709077 to
b7412f4
Compare
|
LGTM |
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM, but left a suggestion - let me know if you want to address that in this PR or as a follow up
There was a problem hiding this comment.
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
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 <[email protected]>
cpuguy83
force-pushed
the
return_ping_data_if_available
branch
from
b7412f4 to
27ef09a
Compare
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
6 participants
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.