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

Personalize callysto singleuser server page #1710

Merged

Conversation

GeorgianaElena
Copy link
Member

@GeorgianaElena GeorgianaElena commented Sep 15, 2022

This uses the singleuser.extraFiles config to add a custom logo for the singleuser server dashboard and notebook, i.e. the Callysto one and hides the Clusters tab from the dashboard.

The Hub interface has been customized too through the https://github.com/2i2c-org/default-hub-homepage/tree/callysto-staging, but only for the staging hub at the moment. I will add a new branch callysto-prod to override the prod hub also.

Closes #1439

Note that I will leave a couple of comments in the code to explain some of the assumptions/compromises I've made.

@@ -32,6 +32,28 @@ jupyterhub:
image:
name: callysto/2i2c
tag: 0.1.0
extraFiles:
tree.html:
Copy link
Member Author

Choose a reason for hiding this comment

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

Normally, extending only the page.html template would have been enough since both the tree and the notebook templates are extending it themselves. But for some reason, it turned impossible to make it to work like this... I fear this is because of NotebookApp -> ServerApp? But I have tried everything with no luck, so this is the best version that's actually doing the job

Comment on lines +41 to +45
<style>
.clusters_tab_link {
visibility: hidden;
}
</style>
Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, actually this one is only possible in "tree.html", so we would have needed two templates anyway

Comment on lines +209 to +211
NotebookApp:
extra_template_paths:
- /usr/local/share/jupyter/custom_template
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'm setting this one in the basehub template because it does no harm if we set it here and we provide no custom templates.

The benefit of this being here is that we can inject extraFiles for other hubs too to extend the templates if need be in the future.

Or at least until will decide on a better strategy as part of #1697 that might imply sync-ing the templates from a repo on the fly like we do with the hub templates.

@GeorgianaElena GeorgianaElena requested a review from a team September 15, 2022 15:05
@GeorgianaElena GeorgianaElena self-assigned this Sep 15, 2022
Copy link
Member

@sgibson91 sgibson91 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@damianavila damianavila left a comment

Choose a reason for hiding this comment

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

Regardless of the specifics of the implementation, I have the feeling we are entering into the user image territory... It should relatively easy for them to self-serve the customization of the templates on the user image itself, right?

@GeorgianaElena
Copy link
Member Author

Thanks for the feedback @sgibson91, @damianavila!

I have the feeling we are entering into the user image territory... It should relatively easy for them to self-serve the customization of the templates on the user image itself, right?

@damianavila , I don't think it has to do with user image, but it's instead a separate thing. And I agree this should be a feature that could be customized by the community instead of us (similar to the hub templates customization).
But as I mentioned in the comments above, this implementation is a compromise. This was my own take on making this to work given the available time and feedback (at that time I considered this to be a time sensitive request, but turns out it wasn't).

I suggest sharing any thoughts about possible ways to implement this in a more sustainable way, in #1697 and consider this an workaround until a clear path forward arises from #1697. WDYT?

In the meantime I will merge this since I have an approval and want to ship this for callysto as promised. But feel free to revert or share any other thoughts about the implementation specifics in #1697. Thanks!

@GeorgianaElena GeorgianaElena merged commit 91b9529 into 2i2c-org:master Sep 28, 2022
@GeorgianaElena GeorgianaElena deleted the callysto-custom-hub-logo branch September 28, 2022 08:49
@github-actions
Copy link

🎉🎉🎉🎉

Monitor the deployment of the hubs here 👉 https://github.com/2i2c-org/infrastructure/actions/runs/3142112735

@damianavila
Copy link
Contributor

damianavila commented Sep 28, 2022

But as I mentioned in the comments above, this implementation is a compromise. This was my own take on making this to work given the available time and feedback (at that time I considered this to be a time sensitive request, but turns out it wasn't).

I understand, thanks for re-raising that point. I think it is a good compromise (you may notice I did not actually request changes to the PR, I just dropped a general opinion).

I suggest sharing any thoughts about possible ways to implement this in a more sustainable way, in #1697 and consider this an workaround until a clear path forward arises from #1697. WDYT?

LGTM, I will reference my comment on that issue!

In the meantime I will merge this since I have an approval and want to ship this for callysto as promised. But feel free to revert or share any other thoughts about the implementation specifics in #1697. Thanks!

No need to revert anything! Thanks again for working on this.

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

Successfully merging this pull request may close these issues.

[New Hub] Callysto
3 participants