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

make image building in hubploy.yaml optional #75

Merged
merged 4 commits into from
Aug 20, 2020

Conversation

scottyhq
Copy link
Contributor

Closes #73

This allows bypassing hubploys image building in favor of an existing image as described here https://zero-to-jupyterhub.readthedocs.io/en/latest/customizing/user-environment.html#choose-and-use-an-existing-docker-image

singleuser:
  image:
    # Get the latest image tag at:
    # https://hub.docker.com/r/jupyter/datascience-notebook/tags/
    # Inspect the Dockerfile at:
    # https://github.com/jupyter/docker-stacks/tree/master/datascience-notebook/Dockerfile
    name: jupyter/datascience-notebook
    tag: 177037d09156

Just added conditionals on the existence of the images: key in hubploy.yaml. If that section does not exist hubploy deploy commands still work. I did not add any error catching if people are to run hubploy build without the images: key

Tested on local hubploy commands for a single hub (https://github.com/escience-pangeo/dssg-jhub). but haven't yet considered how this might affect CI. cc @salvis2 @yuvipanda

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.

Thanks for working on this, @scottyhq! Left a bunch of comments.

hubploy/auth.py Outdated Show resolved Hide resolved
hubploy/auth.py Outdated Show resolved Hide resolved
hubploy/config.py Show resolved Hide resolved
@scottyhq
Copy link
Contributor Author

@yuvipanda - any other changes needed? I'd love to get this merged to fix the linked issue above

@scottyhq scottyhq requested a review from yuvipanda July 29, 2020 17:48
Copy link
Contributor Author

@scottyhq scottyhq left a comment

Choose a reason for hiding this comment

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

I realized i didn't ping you after making the requested changes @yuvipanda . What do you think, ok to merge?

Copy link
Collaborator

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

This LGTM!

- common.yaml
- staging.yaml
- prod.yaml

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm slightly conflicted about re-documenting this structure here. It is helpful, when its kept up to date, but keeping it up to date when its written out in multiple places is a bit messy. Even in this file, its written out on line 110 for example.

I think it's fine to repeat ourselves a bit though, its probably better than assuming its fresh in the reader's mind.

@salvis2
Copy link
Collaborator

salvis2 commented Aug 18, 2020

@scottyhq it looks like you've done what I've done for your deployment and just commented out the build step in the GitHub Action. It feels like we should really make that a method that you can pass through without error if it isn't needed. Ex. if we ran

hubploy build dssg2020 --check-registry --push

and we didn't define images in hubploy.yaml, it should output Building 0 images and return successfully (which it might do, need to test).

We could also enable the images block to specifically hold DockerHub entries. This would need some work in __main__.py, the docs, and hubploy-template. Maybe some new options in config.py.

Is this something you'd be willing to work on as part of this PR, or should we open up another issue and have this be its own PR?

@yuvipanda yuvipanda merged commit 8a55ab0 into berkeley-dsep-infra:master Aug 20, 2020
@yuvipanda
Copy link
Collaborator

Agree it would be nice to have what @salvis2 asked for. Could be a separate PR, though.

@consideRatio the documentation here continues to be a mess, unfortunately... Needs a lot of fixing :(

@salvis2 salvis2 mentioned this pull request Aug 20, 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.

Allow Usage of Pre-Existing Docker Images Instead of Image Building
4 participants