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 bugs uncovered with the increase in test coverage #4

Merged
merged 1 commit into from
Dec 17, 2017

Conversation

theckman
Copy link
Collaborator

  • Potential nil pointer dereference when the outbound HTTP request experienced
    an error (request failed to complete).
  • Mistakenly expecting 403 to be the status code used when the API key provided
    is invalid.

The first issue was that the *http.Response returned from *http.Client.Do()
was being used in the error handling block for that function call to get the
request's URL. This was updated to properly use req.URL.

The second issue was simply catching the wrong HTTP status code on request
processing. This updates the code, and tests, to use http.StatusUnauthorized for
authentication failures.

Signed-off-by: Tim Heckman t@heckman.io

* Potential `nil` pointer dereference when the outbound HTTP request experienced
  an error (request failed to complete).
* Mistakenly expecting 403 to be the status code used when the API key provided
  is invalid.

The first issue was that the `*http.Response` returned from `*http.Client.Do()`
was being used in the error handling block for that function call to get the
request's URL. This was updated to properly use `req.URL`.

The second issue was simply catching the wrong HTTP status code on request
processing. This updates the code, and tests, to use http.StatusUnauthorized for
authentication failures.

Signed-off-by: Tim Heckman <t@heckman.io>
@theckman theckman merged commit 4d334a0 into master Dec 17, 2017
theckman added a commit that referenced this pull request Dec 17, 2017
Fix bugs uncovered with the increase in test coverage

Signed-off-by: Tim Heckman <t@heckman.io>
@theckman theckman deleted the enable_tests branch December 17, 2017 13:20
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.

1 participant