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

Redirect raw.githubusercontent.com URLs as well as raw.github.com #247

Merged
merged 3 commits into from Apr 28, 2014

Conversation

jvns
Copy link
Contributor

@jvns jvns commented Apr 14, 2014

GitHub has started using raw.githubusercontent.com for raw GitHub files as well (or instead?) of raw.github.com. These should be redirected as well.

@rgbkrk
Copy link
Member

rgbkrk commented Apr 14, 2014

Thanks for the contribution!

This could be one regex of the form:

r'/url[s]?/raw\.?github(?:usercontent)?\.com/([^\/]+)/([^\/]+)/(.*)'

This just puts the usercontent piece in a non-capturing group.

@rgbkrk
Copy link
Member

rgbkrk commented Apr 14, 2014

@jvns, would you be willing to fix nbviewer's use of raw URLs too?

@jvns
Copy link
Contributor Author

jvns commented Apr 14, 2014

Done, + added a test. I'm having some trouble running the tests, though -- do they just take a while to run?

@rgbkrk
Copy link
Member

rgbkrk commented Apr 14, 2014

You win 3000 internet points from me for adding a test while you were at it.

The tests do take a little while:

(nbview)14:56:16 (master) ~/code/nbviewer$ invoke test
.........................
----------------------------------------------------------------------
Ran 25 tests in 17.925s

OK

If you dig into them, you'll see a few that request things from the web rather than serving locally. That could be part of it.

@rgbkrk
Copy link
Member

rgbkrk commented Apr 14, 2014

Just fetched your branch and see a few failures. I'll check it out now.

@@ -798,7 +798,7 @@ def get(self, path):
(r'.*/data:.*;base64,.*', Custom404),

(r'/url[s]?/github\.com/([^\/]+)/([^\/]+)/(tree|blob|raw)/([^\/]+)/(.*)', GitHubRedirectHandler),
(r'/url[s]?/raw\.?github\.com/([^\/]+)/([^\/]+)/(.*)', RawGitHubURLHandler),
(r'/url[s]?/raw\.?github(?:usercontent)\.com/([^\/]+)/([^\/]+)/(.*)', RawGitHubURLHandler),
Copy link
Member

Choose a reason for hiding this comment

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

Missing a ? after (?:usercontent).

@rgbkrk
Copy link
Member

rgbkrk commented Apr 14, 2014

After fixing that, all the tests pass except for one that should be completely unrelated to your PR (blob -> tree redirect).

Not sure if our guide lists it, but you should set a few environment variables - GITHUB_OAUTH_KEY and GITHUB_OAUTH_SECRET. Otherwise GitHub may throttle you every now and then.

@rgbkrk
Copy link
Member

rgbkrk commented Apr 14, 2014

I see what you're saying now @jvns. My terminal is hanging after running invoke test now.

@jvns
Copy link
Contributor Author

jvns commented Apr 14, 2014

Yup -- I've been running nosetests, and it hangs no matter what. When I interrupt it, it's inside a time.sleep for some reason

@rgbkrk
Copy link
Member

rgbkrk commented Apr 15, 2014

@jvns - I sent you a PR: jvns#1

Match 0 or 1 usercontent in RawGitHub URL Regular Expression
@rgbkrk
Copy link
Member

rgbkrk commented Apr 23, 2014

Only one of these tests don't pass for me locally, which I think is due to GitHub giving out 400s/rate-limiting us. We need to set this up to either use the encrypted secrets option for Travis with a read only OAuth Key and/or mock GitHub for what should be unit tests.

I'm going to dive into this last issue, see if I can fix it then merge it.

Thanks again @jvns.

@rgbkrk
Copy link
Member

rgbkrk commented Apr 23, 2014

Alright.

https://raw.githubusercontent.com/ipython/ipython/rel-2.0.0/IPython (404s)

Does not behave the same as

https://raw.github.com/ipython/ipython/rel-2.0.0/IPython (redirects)

We can't rely on raw.githubusercontent.com to do a redirect for us and raw.github.com is going away.

@rgbkrk rgbkrk mentioned this pull request Apr 26, 2014
@rgbkrk
Copy link
Member

rgbkrk commented Apr 28, 2014

I'm going to accept this as-is and follow on with a couple patches I have to finish it off.

rgbkrk added a commit that referenced this pull request Apr 28, 2014
Redirect raw.githubusercontent.com URLs as well as raw.github.com
@rgbkrk rgbkrk merged commit 842389c into jupyter:master Apr 28, 2014
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.

None yet

2 participants