-
Notifications
You must be signed in to change notification settings - Fork 68
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
Periodic Docker images build #37
Conversation
This is the current state:
Also installing the systemd units automatically (to be triggered on plugin install) is probably not something we want by default. What do you think @yuvipanda? |
I think we should install it by default. You can put it in https://github.com/voila-gallery/gallery/blob/master/tljh-voila-gallery/tljh_voila_gallery/__init__.py#L61, which is executed only when you install TLJH on a machine by running the bootstrapper. Nothing actually happens if you just install the tljh package, so this won't affect local development. Are rebuilds no-ops if the image is already cached? If so I'd say we should do this every 30min or so instead of 12h. If not, we should make it so and then do it every 30min :D Doesn't need to block this PR though |
Right. Do you think it would also make sense to add a new
I don't think that's the case for now. |
Absolutely! Would love a PR! |
@@ -59,6 +53,7 @@ def login_url(self, base_url): | |||
|
|||
@hookimpl | |||
def tljh_custom_jupyterhub_config(c): | |||
ensure_builder_units() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized this is the wrong place for this. I meant https://github.com/jupyterhub/the-littlest-jupyterhub/blob/master/tljh/hooks.py#L52, which we don't use right now. Should move to a more explicit post-install hook though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. The best is indeed to have a dedicated post install hook (will submit a PR for that soon!).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR to add the tljh_post_install
hook to TLJH: jupyterhub/the-littlest-jupyterhub#389
from ruamel.yaml import YAML | ||
|
||
logging.basicConfig(level=logging.INFO) | ||
|
||
yaml = YAML() | ||
|
||
GALLERY_PATH = 'gallery.yaml' # Relative to module root | ||
GALLERY_URL = 'https://raw.githubusercontent.com/voila-gallery/gallery/master/tljh-voila-gallery/tljh_voila_gallery/gallery.yaml' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you test this locally? What happens when the format of the yaml file changes but not the code? IMO we should (for now) still load it from the local package, and have some method of autodeployment instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, we should of course support the local gallery.yaml. The same gallery file is also used on the front page so when a new example is added it should also be reflected there.
What happens when the format of the yaml file changes but not the code? IMO we should (for now) still load it from the local package, and have some method of autodeployment instead.
Maybe one way to ensure consistency between the code and the gallery.yaml would be to reinstall / upgrade the plugin? Followed by the build of the docker images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased to revert back to reading from the local package.
So to trigger a redeployment of the examples, keeping the timer and systemd unit approach, we could think of:
Both ways would still ensure that |
@jtpio I currently like approach 2 - reinstalling the tljh_voila_gallery package. We can do that in a different PR though. |
|
||
[Timer] | ||
OnBootSec=1min | ||
OnUnitActiveSec=12h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's bring this down to 2h maybe? 12h seems like a long time :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah good catch, I thought it was already lowered. Should be fixed now.
Agree, it can be in a follow-up PR. |
Thanks, @jtpio! |
Thanks for reviewing @yuvipanda! |
Related to #10 and #18 (comment).
For now this can be tested by doing a dev install of TLJH and mounting / installing the plugin in the docker container.
TODO
python -m tljh_voila_gallery.install_build_units
build_images
build fromgallery.yaml
on the master branch (and default to the package resources)Fetch newgallery.yaml
(git pull,requests.get
...) and triggerbuild_images
when the new version has been fetchedShould the systemd units be installed automatically on plugin install? Could be an issue when developing locally.Or runpython -m tljh_voila_gallery.install_build_units
after installing TLJH and the pluginFix-> removeWorkingDirectory
for the systemd unit?install_build_units