Skip to content
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

Error messages should not be masked by matching only on status code #131

Open
helgi opened this issue Jul 16, 2016 · 2 comments
Open

Error messages should not be masked by matching only on status code #131

helgi opened this issue Jul 16, 2016 · 2 comments
Assignees
Labels

Comments

@helgi
Copy link
Contributor

helgi commented Jul 16, 2016

deis/controller#889 is an example of where the return body had a usable error message but got masked by a new custom error message based on the 409 response

@helgi helgi added the bug label Jul 16, 2016
@Joshua-Anderson
Copy link
Contributor

I see two solutions to the problem:

  1. We add error handling for ErrConflict and add a more explicit method. We already do this here
  2. We parse the body of the 409 error message like we do for 400 errors and return more specific errors.

@helgi Does every 409 have a body? I seem to remember the reason why the error handling in the example above was added in the first place was because the body had no message. If that is the case, perhaps the best solution is a mixture of the two, where some specific errors are returned if the 409 has a body, otherwise return an ErrConflict.

@helgi
Copy link
Contributor Author

helgi commented Jul 17, 2016

They should usually have a body but not always. Overall I think we should only be overwriting the message from the API if the message is a boilerplate message, such as Not Found or Conflict.

I believe even the one you linked to in the first bullet point has a proper body that shouldn't need to be overwritten. That part of the SDK could be changed as well then

@Joshua-Anderson Joshua-Anderson self-assigned this Jul 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants