-
Notifications
You must be signed in to change notification settings - Fork 18.9k
Make client return "rich" errors (take 2) #38689
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #38689 +/- ##
=========================================
Coverage ? 36.48%
=========================================
Files ? 614
Lines ? 45876
Branches ? 0
=========================================
Hits ? 16737
Misses ? 26857
Partials ? 2282 |
|
discussing in the maintainers meeting; we can move the http utilities to the errdefs package |
|
ping @cpuguy83 is this what you meant (at least for now?); I can work on a follow-up to preserve the statuscode (without the interface) |
api/server/httputils/errors.go
Outdated
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.
Did we mention we'd move this to errdefs?
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.
ach, yes, slipped my mind; let me work in that one
client/checkpoint_delete_test.go
Outdated
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.
Not sure I understand these checks here. We have a mock client and are testing that the mock does what we just told it to do?
Seems strange... of course I know you didn't write this.
|
Looks good. |
ed51eb2 to
173faf3
Compare
|
ping @cpuguy83 I moved the httputils (except for |
173faf3 to
be0be67
Compare
client/errors.go
Outdated
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.
Actually, thinking of reverting this change; I already updated the IsNotFound() utility to match both this error, and the errdefs.NotFound() interface, so perhaps we should keep this one (for now) to make both the old and new ones work.
@cpuguy83 wdyt?
|
|
Argh. That test looks weird; it's expecting an error to be produced for a non-error (204) status 🤔 |
76dc0d8 to
cbc1d4f
Compare
|
For now, reverted that part in the last commit, but added TODO's instead (as it looks incorrect) // TODO this code converts non-error status-codes (e.g., "204 No Content") into an error; verify if this is the desired behavior
if response.statusCode != http.StatusOK {
return nil, types.ContainerPathStat{}, fmt.Errorf("unexpected status code from daemon: %d", response.statusCode)
} |
|
Interesting; failures on |
Ah! Found what happened 😂 My IDE tried to be helpful and decided to add an import for |
cbc1d4f to
4848e07
Compare
4848e07 to
4c155ef
Compare
This utility allows a client to convert an API response back to a typed error; allowing the client to perform different actions based on the type of error, without having to resort to string-matching the error. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
this patch makes the client return errors matching the errdefs interface. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
looks like we don't need this handling
Before this patch:
Error: No such image: nosuchimage
After this patch:
Error response from daemon: No such image: nosuchimage:latest
"
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
the containerd errdefs functions have the same name as the docker errdefs, but their types use a different signature; use an alias to prevent them from being mistaken for the docker errdefs equivalents. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
042e4f7 to
818d0dc
Compare
|
Updated the comment; PTAL |

closes #38467
Alternative to #38467, but without preserving the original status code (only transforming status codes back to either a known
errdefor to a "generic" system/invalidrequest errorThe API client currently discards HTTP-status codes that are returned from the daemon. As a result, users of the API client that want to implement logic based on the type of error returned, will have to resort to string-matching (which is brittle).
This PR is a first attempt to make the client return more useful errors;
errdeferrors, so that it's easy to check the type of error (errdef.IsNotFound(err),errdef.IsConflict(err))With this patch, something like the below will be possible;
Feedback needed