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

Temporary page while tljh is building #605

Merged
merged 21 commits into from Sep 14, 2020

Conversation

GeorgianaElena
Copy link
Member

@GeorgianaElena GeorgianaElena commented Aug 3, 2020

This shows a temporary page while tljh is building, that looks like this:
tljh-is-building-page

A few things to consider:

  • There will be a "down" period between when this page stops showing and JupyterHub starts, because we have to free up the port for traefik.
  • The /logs page has to be manually refreshed to update the logs
  • The main page gets refreshed every 30s so that when JH starts, the login page will be shown instead

TODO

  • Add / update documentation
  • Add / update tests

Closes #70

Copy link
Collaborator

@yuvipanda yuvipanda left a comment

Choose a reason for hiding this comment

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

This is amazing work, @GeorgianaElena! \o/ Absolutely awesome, thank you for picking this up.

The pid file is mostly used to just pass the pid of the running server from the bootstrapper to the installer, right? If so, can you pass it as a parameter to the installer, via https://github.com/jupyterhub/the-littlest-jupyterhub/pull/605/files#diff-3246cd4be92bfdc6105293969b81c0d9L177 or similar, instead of using a file to communicate? Probably simpler.

I also wonder if this should be behind password protection, in case the logs have sensitive information?

We should also put this behind a flag, so we can turn it on / off as we wish.

Either way, amazing work. Look forward to merging this :)

bootstrap/bootstrap.py Outdated Show resolved Hide resolved
@GeorgianaElena
Copy link
Member Author

Thanks for the feedback @yuvipanda :)
I addressed all of the suggestions, except for the password one.

I also wonder if this should be behind password protection, in case the logs have sensitive information?

There shouldn't be anything sensitive in the logs, sice we're only logging generic messages at each step. I think there might be some in case errors occur, but those go only to stderr and are not stored into installer.log.

But if we decide to password protect the temporary page, maybe one way would be to use the user and password of the first tljh admin user, that are passed to the bootstrap script.

What do you think?

Copy link
Collaborator

@yuvipanda yuvipanda left a comment

Choose a reason for hiding this comment

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

A bunch more comments! This is looking good :)

In general, I think calling this 'install progress page' or some such might be more descriptive than 'temporary page'. What do you think?

Excited to get this going :)

bootstrap/index.html Outdated Show resolved Hide resolved
bootstrap/index.html Outdated Show resolved Hide resolved
tljh/installer.py Outdated Show resolved Hide resolved
tljh/installer.py Outdated Show resolved Hide resolved
bootstrap/bootstrap.py Outdated Show resolved Hide resolved
bootstrap/bootstrap.py Outdated Show resolved Hide resolved
bootstrap/bootstrap.py Outdated Show resolved Hide resolved
@yuvipanda
Copy link
Collaborator

But if we decide to password protect the temporary page, maybe one way would be to use the user and password of the first tljh admin user, that are passed to the bootstrap script.

Yep, that's the way to do it I think! But we can add that as a separate PR, not necessarily part of this one.

@yuvipanda
Copy link
Collaborator

what kinda tests do you think we should add to this?

@GeorgianaElena
Copy link
Member Author

I added a test in test_bootrap.py that checks if there is something at /index.html while tljh is building and that the same path returns a 404 once jupyterhub starts.

Do you think this is a good enough test @yuvipanda?

@yuvipanda
Copy link
Collaborator

Things look good to me, @GeorgianaElena! I left some minor questions. Once we get the tests to pass this can be merged! \o/

@GeorgianaElena
Copy link
Member Author

Tests are now passing @yuvipanda, if you want to take another look 🎉

@yuvipanda
Copy link
Collaborator

The upgrade test is failing thanks to jupyterlab again, so I'm just going to merge this.

AWESOME WORK, @GeorgianaElena! \o/

@yuvipanda yuvipanda merged commit f0b8745 into jupyterhub:master Sep 14, 2020
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.

Serve a temporary "TLJH is building" page while TLJH is building
2 participants