feat(image registry): allow configuring another image registry #479
Conversation
|
@darnaut or @david-leifker would be great if you have time for some review :) |
DSchmidtDev
force-pushed
the
feat/global_container_registry
branch
from
3f8d229
to
376d51c
Compare
|
This PR is stale. We will close it in 30 days if there is no comment or activity. If you want feedback but not able to get it on github please head to #contribute channel in slack at https://slack.datahubproject.io. |
|
comment to keep it active. Maybe someone please review? :) |
DSchmidtDev
force-pushed
the
feat/global_container_registry
branch
from
90178d0
to
4d5a2e4
Compare
| @@ -61,7 +61,8 @@ spec: | |||
| - name: {{ .Chart.Name }} | |||
| securityContext: | |||
| {{- toYaml .Values.securityContext | nindent 12 }} | |||
| image: "{{ .Values.image.repository }}:{{ required "Global or specific tag is required" (.Values.image.tag | default .Values.global.datahub.version) }}" | |||
| {{- $registry := .Values.global.imageRegistry | default .Values.image.registry -}} | |||
There was a problem hiding this comment.
Can we add this as helpers (e.g. charts/datahub/templates/_helpers.tpl) instead of duplicating the logic across many templates?
There was a problem hiding this comment.
Thanks for the feedback. I added the usage of templates. Unfortunately especially the datahub-upgrade jobs don't use the same mechanisms and structuring like subcharts so I simply duplicated the templates.
One idea would be to re-map the variables like a context but I am not familiar with it yet and do not want to blow up this PR more.
But would be a refactoring to do after that?
There was a problem hiding this comment.
Looking better. I provided an example that removes the need to duplicate templates.
DSchmidtDev
force-pushed
the
feat/global_container_registry
branch
from
54a7fc6
to
4b7c990
Compare
DSchmidtDev
force-pushed
the
feat/global_container_registry
branch
from
471caa1
to
f6f10aa
Compare
| Create image registry, name and tag for mysql setup job | ||
| */}} | ||
| {{- define "mysqlSetupJob.image" -}} |
There was a problem hiding this comment.
No need to duplicate for each image, just have a generic one like:
{{/*
Create image registry, name and tag for a datahub component
*/}}
{{- define "datahub.image" -}}
{{- $registry := .imageRegistry | default .image.registry -}}
{{ $registry }}/{{ .image.repository }}:{{ required "Global or specific tag is required" (.image.tag | default .version) -}}
{{- end -}}
and include it as:
image: {{ include "datahub.image" (dict "imageRegistry" .Values.global.imageRegistry "version" .Values.global.datahub.version "image" .Values.image) }}
There was a problem hiding this comment.
ah very nice. Did not know this trick :)
charts/datahub/values.yaml
Outdated
| @@ -123,6 +123,7 @@ datahub-ingestion-cron: | |||
| elasticsearchSetupJob: | |||
| enabled: true | |||
| image: | |||
| registry: docker.io | |||
There was a problem hiding this comment.
wouldn't it be better to just set global.imageRegistry? This way image.registry would be used for overrides.
There was a problem hiding this comment.
I thought to leave it at the lowest level so every job itself is consistent to use docker.io if someone does not adapt anything. But if one wants to use e.g. a mirror it just needs to be overridden globally.
But you are right, if its set globally it takes precedence over the local config. I may need to adapt the template as well. Which way do you prefer? I can adapt it according to it
There was a problem hiding this comment.
In the latest commit I adapted the order. What do you think about?
There was a problem hiding this comment.
Looks good to me. I suspect most users wanting to change the registry would want to do so for all images (e.g. their cluster does not have access to docker-hub/internet), otherwise it's to overwrite a specific image.
DSchmidtDev
force-pushed
the
feat/global_container_registry
branch
from
a75c444
to
98511c7
Compare
This PR adds the ability to configure globally another image registry like your personal docker mirror.
If no global registry is set the default one configured in the service itself is used.
Checklist
Links to related issues (if applicable)Tests for the changes have been added/updated (if applicable)