-
Notifications
You must be signed in to change notification settings - Fork 66
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
Add cloudbuild configuration for automatic docker image builds #25
Conversation
Codecov Report
@@ Coverage Diff @@
## master #25 +/- ##
==========================================
- Coverage 45.78% 45.74% -0.04%
==========================================
Files 17 17
Lines 2728 2728
==========================================
- Hits 1249 1248 -1
- Misses 1479 1480 +1
Continue to review full report at Codecov.
|
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.
Nice, few comments!
scripts/cloudbuild.py
Outdated
def tag(self) -> str: | ||
'''returns tag for this spec''' | ||
parts = [] | ||
if self.gpu_version is None: # cpu image |
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.
Can we pull this logic out into a separate function, and return some enum value? It is always easier for me to read these conditionals when they're simply defining a mode. Then we can switch-case on the mode inside here (and anywhere else that needs it).
scripts/cloudbuild_config.json
Outdated
"py38" : ["PYTHON_VERSION=3.8", "MINICONDA_VERSION=py38_4.8.2"] | ||
}, | ||
"images" : [ | ||
{"cpu" : "py37"}, |
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.
why is the python version a value, keyed by cpu
here?
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.
Is the concern here that the 'cpu' key is misleading? Easy enough to rename this to 'python' if that helps.
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.
Updates for last review.
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.
One fix, then this is good!
dockerfiles/Dockerfile
Outdated
# minicoda release archive is here: https://repo.anaconda.com/miniconda | ||
# see the docs here for managing python versions with conda: | ||
# https://docs.conda.io/projects/conda/en/latest/user-guide/tasks/manage-python.html | ||
ARG MINICONDA_VERSION=py38_4.8.2 |
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.
needs to be py37
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, will fix this.
Please have a look. This adds a cloudbuild.json file that configures cloud build for our docker base images. This also adds a script to generate this file, and a configuration file for setting up our cloud builds.
The config file in the PR has the url set to a test value for validation before we enable this for automatic builds.