Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Conversation

@shana
Copy link
Contributor

@shana shana commented Sep 18, 2015

Let's not merge this one quite yet, it's not needed for the next release.

Copy link
Contributor

Choose a reason for hiding this comment

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

So this isn't needed any more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's deprecated and ignored due to redirect work, says the comment coming from this commit: octokit/octokit.net@3acdd10. The code analysis bails on deprecation warnings, so I took all the calls out.

I have not tested everything, so we might want to hold on merging this for now.

Copy link
Member

Choose a reason for hiding this comment

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

This is a side-effect of the repository redirects from the API, and because AllowAutoRedirect doesn't set the Authorization header citation when redirecting, it would break for most of our scenarios.

The Authorization header is cleared on auto-redirects and HttpWebRequest automatically tries to re-authenticate to the redirected location. In practice, this means that an application can't put custom authentication information into the Authorization header if it is possible to encounter redirection.

We're now handling the redirects in Octokit and setting the right authorization headers. See octokit/octokit.net#808 for the whole backstory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the info @shiftkey!

@shana shana changed the title Update octokit to 0.16.0 [wip] Update octokit to 0.16.0 Sep 18, 2015
@shana shana changed the title [wip] Update octokit to 0.16.0 Update octokit to 0.16.0 Oct 5, 2015
@shana
Copy link
Contributor Author

shana commented Oct 5, 2015

We're good to go on this one, redirects are handled internally in HttpClient in a proper fashion so we don't have to worry about that 🍍

haacked added a commit that referenced this pull request Oct 5, 2015
@haacked haacked merged commit b69935d into master Oct 5, 2015
@haacked haacked deleted the shana/update-octokit branch October 5, 2015 17:52
@haacked
Copy link
Contributor

haacked commented Oct 5, 2015

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants