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

Follow redirects in GitHubPages::HealthCheck#served_by_pages? #13

Merged
merged 4 commits into from
Apr 15, 2015

Conversation

jdennes
Copy link
Member

@jdennes jdennes commented Apr 13, 2015

Many apex domains like http://getbootstrap.com/ use A records (rather than ALIAS/ANAME records):

› dig getbootstrap.com
;getbootstrap.com.      IN  A
getbootstrap.com.   9072    IN  A   192.30.252.154
getbootstrap.com.   9072    IN  A   192.30.252.153

GitHub Pages will sometimes respond with a 302 Found for these domains:

› curl -I http://getbootstrap.com/
HTTP/1.1 302 Found
Location: /

Previously the GitHubPages::HealthCheck#served_by_pages? check could fail because it would make a request which would respond with a 302 Found and the Server: github.com header wouldn't be included in the response.

This fixes the issue by retrying the request, which should then result in a 200 OK response which includes the Server: github.com header.

/cc @benbalter

jdennes added 3 commits April 13, 2015 18:07
An apex domain using A records will sometimes return a 302 Found
response, which redirects to the same location.
@jdennes jdennes changed the title Follow redirects once in GitHubPages::HealthCheck#served_by_pages? Follow redirects in GitHubPages::HealthCheck#served_by_pages? Apr 13, 2015
@spraints
Copy link
Member

It looks like it doesn't actually follow the redirect, but instead requests the same URL again. That should be OK in this case, since it ends up making the same request.

👍

@jdennes
Copy link
Member Author

jdennes commented Apr 13, 2015

@spraints Yes, correct. I have run script/cibuild quite a few times to test this out and it was still occasionally failing with the single retry. I've just made a slight change to retry a maximum of three times. What do you think? Doesn't look like CI is set up correctly for this repository currently...

@benbalter
Copy link
Contributor

Nice catch. 👍

@spraints
Copy link
Member

Doesn't look like CI is set up correctly for this repository currently...

This repository is public. IIRC our CI will not run for public repositories. Maybe we should set up a travis CI build for this?

@jdennes
Copy link
Member Author

jdennes commented Apr 15, 2015

Maybe we should set up a travis CI build for this?

I think it was intended for script/cibuild to be run on Travis CI. Not sure why it's not currently.

jdennes added a commit that referenced this pull request Apr 15, 2015
Follow redirects in GitHubPages::HealthCheck#served_by_pages?
@jdennes jdennes merged commit a496512 into master Apr 15, 2015
@jdennes jdennes deleted the jdennes-follow-redirects branch April 15, 2015 10:48
@benbalter
Copy link
Contributor

I think it was intended for script/cibuild to be run on Travis CI. Not sure why it's not currently.

When the Gem was released it was when we were testing some of the new Org Oauth Whitelisting which is now public. There was a hiccup with Travis's ability to properly setup the webhook. Looks like it's live now. Thanks @jdennes!

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.

3 participants