behaviour: fix downloading assets with redirects #98
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
After doing more thorough acceptance on #97, I realized that the HTTP request was not using the OAuth HTTP client if the access token was provided (i.e. if an access token is provided, the initial request will use it, but if the initial response is a redirect, the subsequent request will be unauthenticated). Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>
fixes #92 Previously, we were just downloading the API response to a file since we did not specify the `Accept` header (as documented here: https://developer.github.com/v3/repos/releases/#get-a-single-release-asset). Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>
When trying to download release assets that redirected to e.g. AWS S3, my prior commits would cause an error. Since we were using the OAuth client Transport as an `http.RoundTripper`, we were always adding the `Authorization` header (overwriting Go's default behaviour of stripping the Authorization header on redirects). To avoid this, just use a vanilla HTTP client and pass the access token manually. If the initial redirect URL is external to github, check the Host explicitly to determine whether to add the access token. On subsequent redirects, this is taken care of by Go's http client stripping the Authorization header. I did consider creating a custom `http.RoundTripper` to strip the Authorization header for any request outside of github.com, but that felt like overkill here. Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>
|
it appears as though it works! :) |
|
@jomsie no rush, but do you have further reviewing to do, or is this good to be merged? |
I was just finishing up reviewing the code :D |
|
@jomsie whoops, forgive my impatience! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses two issues around downloading assets that provide redirect URLs:
Acceptheader toapplication/octet-streamas documented here