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

fix: report http error if /api call fails #3702

Merged
merged 1 commit into from
Oct 10, 2019
Merged

Conversation

bboreham
Copy link
Collaborator

@bboreham bboreham commented Oct 6, 2019

Previously it would try to run the JSON decoder on a string like "404 not found" and report that failing with the message "Error fetching app details: only encoded map or array can be decoded into a struct".

Now it will report the http error, like this:

<probe> ERRO: 2019/10/06 17:00:42.290708 Error fetching app details: Error response from http://172.16.0.3:4040//api: 404 Not Found

Fixes #3249

Previously it would try to run the JSON decoder on a string like "404
not found" and report that failing.
Copy link
Contributor

@fbarl fbarl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (except the small nit I commented on) ✔️

Fixes #3249

I think this relies on the assumption that /api call failing is sole reason of JSON decoding failing - is there a chance /api would ever successfully return a badly formatted JSON?

I just want to make sure the message in #3249 will be completely eliminated before we close it :)

@@ -192,6 +192,9 @@ func (c *appClient) Details() (xfer.Details, error) {
if err != nil {
return result, err
}
if resp.StatusCode != http.StatusOK {
return result, fmt.Errorf("Error response from %s: %s", c.url("/api"), resp.Status)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: based on your own log output (Error response from http://172.16.0.3:4040//api: 404 Not Found), maybe there's an extra / here:

Suggested change
return result, fmt.Errorf("Error response from %s: %s", c.url("/api"), resp.Status)
return result, fmt.Errorf("Error response from %s: %s", c.url("api"), resp.Status)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The output is correct: the reason it's failing is the url has too many slashes in it.

@bboreham
Copy link
Collaborator Author

bboreham commented Oct 8, 2019

#3249 has (like many of our issues) multiple people reporting multiple sets of symptoms.
As far as I can see most of the people commenting subsequently realised it was an addressing problem, which this change will help them with.
People who still have problems can open a new issue.

Copy link
Contributor

@qiell qiell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@bboreham bboreham merged commit 43f6b96 into master Oct 10, 2019
@bboreham bboreham deleted the report-api-details-error branch October 10, 2019 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error fetching app details: only encoded map or array can be decoded into a struct
3 participants