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

[MRG] Check if nbviewer URL would show an error #934

Merged
merged 2 commits into from
Aug 20, 2019

Conversation

betatim
Copy link
Member

@betatim betatim commented Aug 20, 2019

If the nbviewer widget would show a 404 or other error we don't want to
display it. We check the status before rendering the template by making
a HEAD request to nbviewer, if the status code is >=400 we don't show
the preview.

closes #922

@betatim
Copy link
Member Author

betatim commented Aug 20, 2019

@lesteve I only saw your comment after making this PR. Sorry :-/

This implements your idea using the tornado HTTP client instead of urllib. The code has to work async/non-blocking and I don't think urllib supports that (yet).

It isn't quite the feature requested in #816 because it doesn't give the choice of disabling or not to the user. However this is a quick win, we can still add the "disable even if the preview works" options if we decide we want that.

If the nbviewer widget would show a 404 or other error we don't want to
display it. We check the status before rendering the template by making
a HEAD request to nbviewer, if the status code is >=400 we don't show
the preview.
@lesteve
Copy link

lesteve commented Aug 20, 2019

Very nice to see this, thanks a lot!

No worries about superseding my naive attempt. I got binder setup locally using minikube and learned a few useful things along the way!

@betatim betatim merged commit 0216360 into jupyterhub:master Aug 20, 2019
@betatim betatim deleted the no-preview-on-404 branch August 20, 2019 14:01
yuvipanda pushed a commit to jupyterhub/helm-chart that referenced this pull request Aug 20, 2019
@lesteve
Copy link

lesteve commented Aug 20, 2019

Very nice to see this merged! Naive question: I assume I have to monitor the commits in https://github.com/jupyterhub/mybinder.org-deploy in particular the changes in values.yaml (I have read the blog post https://blog.jupyter.org/automating-mybinder-org-dependency-upgrades-in-10-steps-bb5e38542059).

Are there additional steps I need to be aware of until this change reach mybinder.org?

Note: I am not in a hurry at all, I am asking out of curiosity and to have some very rough time estimate when I can give this a whirl on mybinder.org.

@betatim
Copy link
Member Author

betatim commented Aug 20, 2019

We have a bot (henchbot, just referenced this PR) that opens a PR on the deploy repo. The bot runs about once an hour and usually(depends on where people are, if it is night, weekend, etc) someone clicks merge an hour or two after the bot opened the deploy PR. Then it is live!

@lesteve
Copy link

lesteve commented Aug 20, 2019

OK that matches what I understood from the blog post, thanks for confirming this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

404 page shown during build when pointing to a repo
3 participants