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] Make the URL used to generate launch badges configurable #859

Merged
merged 1 commit into from
Jun 2, 2019

Conversation

betatim
Copy link
Member

@betatim betatim commented May 25, 2019

This lets you set the base URL used for generating badges. This is useful when you have a group of hubs at different hostnames but want badges generated on all of them to point to a common hostname. I think we should use this for operating the GKE and OVH cluster as a federation so that all clusters generate https://mybinder.org/ badges and launch URLs.

@betatim betatim changed the title Make the URL used to generate launch badges configurable [MRG] Make the URL used to generate launch badges configurable May 25, 2019
@betatim betatim requested a review from choldgraf May 28, 2019 05:08
@@ -15,6 +15,7 @@ class MainHandler(BaseHandler):
def get(self):
self.render_template(
"index.html",
badge_base_url=self.settings['badge_base_url'],
Copy link
Collaborator

@bitnik bitnik May 28, 2019

Choose a reason for hiding this comment

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

Suggested change
badge_base_url=self.settings['badge_base_url'],
badge_base_url=self.settings['badge_base_url'] or
f"{self.request.protocol}://{self.request.host}{self.settings['base_url']}",

maybe this is better. so you can remove if elses in js code and it becomes cleaner.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure which one I prefer. I agree the JS has a few extra if statements, but I think I prefer it that way because it leaves the choice to the JS code. I dunno, it is just a feeling :-/

@choldgraf
Copy link
Member

This looks great to me - the one thing that's missing is any documentation about this feature 📚 . I guess it would show up here: https://binderhub.readthedocs.io/en/latest/reference/app.html maybe that's enough?

@betatim
Copy link
Member Author

betatim commented May 30, 2019

Nods that is where you'd find an entry.

We could write a short how to section on setting up a federation? I think that is the only context in which you'd want to use this feature.

@choldgraf
Copy link
Member

@betatim just to double check are you planning on putting off the docs for this for another PR? (in which case I can open an issue about it so we don't forget)

@betatim
Copy link
Member Author

betatim commented Jun 2, 2019

It should appear automatically in the reference section of the docs (preview). Not sure any other documentation needs writing.

I'd make writing "howto setup a federation" a new issue as that will require a bit of thinking (for example what redirector should people use).

@choldgraf choldgraf merged commit 0d300b8 into jupyterhub:master Jun 2, 2019
@choldgraf
Copy link
Member

then thanks for the contribution @betatim !

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

Successfully merging this pull request may close these issues.

3 participants