Parameterize Dockerfiles for Open edX releases #3864
Conversation
| @@ -8,7 +8,8 @@ | |||
| # with the currently checked-out configuration repo. | |||
|
|
|||
|
|
|||
| FROM edxops/xenial-common:latest | |||
| ARG IMAGE_PREFIX | |||
There was a problem hiding this comment.
Is the default for this just an empty string? What are other valid values?
There was a problem hiding this comment.
Yes, it defaults to empty string. I'm not sure the best way to indicate that. It works now if not provided.
There was a problem hiding this comment.
Let's add a comment just so that clear. Also is there anything we can say about how to figure out what are valid values for this arg? It's fine if there isn't but would be useful to put something here if there is.
There was a problem hiding this comment.
What are some example values?
There was a problem hiding this comment.
@clintonb I believe the idea is to have Open edX specific prefixes, e.g. ficus-.
There was a problem hiding this comment.
I should have explained more, sorry. Master would make "edxops/edxapp:latest", and Ficus would make "edxops/ficus/edxapp:latest". $IMAGE_PREFIX would be "ficus/"
There was a problem hiding this comment.
I've added the description to the top of the PR.
There was a problem hiding this comment.
@nedbat any reason not to put that info in this file or a README under the docker/build directory?
There was a problem hiding this comment.
I've added details to the docker/README.md (i'll convert to .rst in a follow-on).
| @@ -8,7 +8,8 @@ | |||
| # with the currently checked-out configuration repo. | |||
|
|
|||
|
|
|||
| FROM edxops/xenial-common:latest | |||
| ARG IMAGE_PREFIX | |||
There was a problem hiding this comment.
What are some example values?
| @@ -1,5 +1,5 @@ | |||
| FROM ubuntu:xenial | |||
| MAINTAINER edxops | |||
| MAINTAINER edxops | |||
There was a problem hiding this comment.
MAINTAINER instruction is deprecated: https://docs.docker.com/engine/reference/builder/#maintainer-deprecated
There was a problem hiding this comment.
I thought the maintainer was supposed to be an email address, but I can't find where I read that, and I don't know what email we would use. I'll leave this to a following PR.
| @@ -10,7 +10,8 @@ ENV LANG C.UTF-8 | |||
|
|
|||
| ENV ANSIBLE_REPO="https://github.com/edx/ansible" | |||
| ENV CONFIGURATION_REPO="https://github.com/edx/configuration.git" | |||
| ENV CONFIGURATION_VERSION="master" | |||
| ARG OPENEDX_RELEASE=master | |||
| ENV CONFIGURATION_VERSION="$OPENEDX_RELEASE" | |||
There was a problem hiding this comment.
Can we standardize on ${var} syntax?
nedbat
force-pushed
the
nedbat/parameterize-dockerfiles
branch
from
156967e
to
9c7242e
Compare
| @@ -8,7 +8,8 @@ | |||
| # with the currently checked-out configuration repo. | |||
|
|
|||
|
|
|||
| FROM edxops/xenial-common:latest | |||
| ARG IMAGE_PREFIX | |||
There was a problem hiding this comment.
I've added details to the docker/README.md (i'll convert to .rst in a follow-on).
Configuration Pull Request
Add some parameterization to the Dockerfiles so we can make images for Open edX releases. (Much more is needed to complete that work, this is a start).
This provides for choosing a different branch other than master ($OPENEDX_RELEASE), and for adding a component to the image names ($IMAGE_PREFIX).
The IMAGE_PREFIX would be empty for master builds, so the images will be named as they are today: "edxops/edxapp:latest". For Ficus, IMAGE_PREFIX would be "ficus/", so the image would be "edxops/ficus/edxapp:latest".
These files use a feature newly available in Docker 17.05: the use of ARGS in the FROM line.
Make sure that the following steps are done before merging: