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

Display the specific error in the case of a 403 #710

Merged
merged 2 commits into from Oct 19, 2015
Merged

Display the specific error in the case of a 403 #710

merged 2 commits into from Oct 19, 2015

Conversation

rubyist
Copy link
Contributor

@rubyist rubyist commented Oct 6, 2015

A status of 403 against the API would be masked with a generic message. This displays a more specific message - either one returned in the API json, or a default message about authentication.

@technoweenie
Copy link
Contributor

Should we just do this with every status code that falls through? I think returning the err would be better than losing it here:

    if res.StatusCode != 200 {
        return nil, Error(fmt.Errorf("Invalid status for %s %s: %d", req.Method, req.URL, res.StatusCode))
    }

Assuming that the response has valid json with a "message" property, the error should get this info here.

@rubyist
Copy link
Contributor Author

rubyist commented Oct 6, 2015

Yeah that makes sense. That latest commit handles all the fallthroughs. The changed test shows that if the call returns some weird status without json it'll display an error about being unable to parse the response.

@rubyist rubyist added the review label Oct 7, 2015
technoweenie added a commit that referenced this pull request Oct 19, 2015
Display the specific error in the case of a 403
@technoweenie technoweenie merged commit e08632f into master Oct 19, 2015
@technoweenie technoweenie deleted the 403 branch October 19, 2015 19:20
@technoweenie technoweenie mentioned this pull request Oct 19, 2015
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants