-
Notifications
You must be signed in to change notification settings - Fork 14
feat: Allow to override the base image for builds #96
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
Conversation
c823a08
to
c1c2064
Compare
{% if DOCKER_REGISTRY_PROXY is defined %} | ||
ARG BASE_IMAGE={{ DOCKER_REGISTRY_PROXY }}/ubuntu:22.04 | ||
{% else %} | ||
ARG BASE_IMAGE=docker.io/ubuntu:22.04 | ||
{% endif %} | ||
|
||
FROM ${BASE_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.
Actually, this looks a bit too complicated to me. I'd suggest this instead:
{% if DOCKER_REGISTRY_PROXY is defined %} | |
ARG BASE_IMAGE={{ DOCKER_REGISTRY_PROXY }}/ubuntu:22.04 | |
{% else %} | |
ARG BASE_IMAGE=docker.io/ubuntu:22.04 | |
{% endif %} | |
FROM ${BASE_IMAGE} | |
FROM {{ BACKUP_BASE_IMAGE }} |
... and then a corresponding change in plugin.py
. If someone wanted to pull their Ubuntu image from a different source, they could just override BACKUP_BASE_IMAGE
. Wouldn't that simplify things?
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.
Sure, that would be better from one plugin perspective. My thought was that if you want to proxy the base images for x number of plugins, it would be better to add a shared variable to the deployment instead of x number of per plugin variables.
Additionally, if we allow to override the base image version, wouldn't that suggest that this plugin should work as expected with any version? While really we are bumping the base image versions based on the supported versions in the upstream releases of Tutor and Open edX and tagging the plugin accordingly.
But, I might very well be overthinking it :) Should I go ahead and apply your suggested approach to all plugins?
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.
Sure, that would be better from one plugin perspective. My thought was that if you want to proxy the base images for x number of plugins, it would be better to add a shared variable to the deployment instead of x number of per plugin variables.
You're not wrong, but in that case that would be a variable that shouldn't be added at the plugin level — rather, it should be added upstream in Tutor, and then consumed by downstream repos. You can certainly try to submit such a patch. In the past Régis hasn't been particularly inclined towards accepting such changes, but he may have modified his stance since.
Additionally, if we allow to override the base image version, wouldn't that suggest that this plugin should work as expected with any version?
No, I'd argue that it wouldn't. The general expectation is that the plugin works with its defaults applied. If a user overrides a variable for which we supply a default, and it breaks, it's for them to fix that.
Compare this: if a user were to set BACKUP_K8S_CRONJOB_BACKUP_SCHEDULE
to an invalid cron expression, or BACKUP_K8S_CRONJOB_CONCURRENCYPOLICY
to a keyword that Kubernetes does not understand, then that would be an incorrect configuration, but not one that you, the plugin author, are responsible for. If you provided defaults for these variables that can't work, then you would be.
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.
Ack, fair enough, thank! I will go ahead and make the suggested changes.
tutorbackup/plugin.py
Outdated
@@ -16,6 +16,7 @@ | |||
config = { | |||
"defaults": { | |||
"VERSION": __version__, | |||
"DOCKER_REGISTRY_PROXY": None, |
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.
"DOCKER_REGISTRY_PROXY": None, | |
"BASE_IMAGE": "docker.io/ubuntu:22.04", |
c1c2064
to
7226a6e
Compare
Add a config option `BACKUP_BASE_IMAGE` that allows to override the base image for builds.
7226a6e
to
4e8d171
Compare
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.
Looks good to me!
Add a config option
BACKUP_BASE_IMAGE
that allows to override the base image for builds.