Handle JSON errors from the server#233
Handle JSON errors from the server#233thaJeztah merged 1 commit intodocker-archive-public:masterfrom
Conversation
bfirsh
changed the title
bfirsh
force-pushed
the
handle-json-errors
branch
from
ec14b70 to
78dbbce
Compare
|
Hmm, I'm not sure about this. |
|
This is also importing packages from |
|
@cpuguy83 There is a need for JSON errors of some kind, no strong feelings about whether there's a code or not. Some discussion of that in moby/moby#22880 @calavera Ah, understood. I'll put the error struct in here. |
bfirsh
force-pushed
the
handle-json-errors
branch
from
78dbbce to
aab2dfe
Compare
|
Added |
client/request.go
Outdated
| return serverResp, fmt.Errorf("Error response from daemon: %s", bytes.TrimSpace(body)) | ||
|
|
||
| var errorMessage string | ||
| if strings.Contains(resp.Header.Get("Content-Type"), "application/json") { |
There was a problem hiding this comment.
should this be ==? what are the possible content types? we should check specifically for them.
There was a problem hiding this comment.
The server returns application/json; charset=utf-8, and the default is utf-8 so we should support just application/json too.
Realistically we'll only consume/produce utf-8, so perhaps this should check for the exact strings application/json or application/json; charset=utf-8.
There was a problem hiding this comment.
Hmm. Seems like I'm wrong and the Engine actually just returns application/json. Not sure where I found charset=utf-8. Perhaps it was in some documentation somewhere.
I've updated to just check for the exact string application/json.
Signed-off-by: Ben Firshman <[email protected]>
bfirsh
force-pushed
the
handle-json-errors
branch
from
aab2dfe to
a053269
Compare
|
LGTM |
|
Tested this and it seems to behave as I would expect with different API versions. LGTM (I would prefer to add the error code, but the whole idea is this is extensible later eg we can add stack traces in debug mode or line the error is from, or nested error info from runc etc later so lets go with this now). |
|
moving this to code review; or were we there already? |
|
Yes, code LGTM. |
|
LGTM |
7 participants
See moby/moby#22880