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

Add top level registry prefix to Helm Charts for custom registries #1510

Closed
axsaucedo opened this issue Mar 5, 2020 · 3 comments · Fixed by #1517
Closed

Add top level registry prefix to Helm Charts for custom registries #1510

axsaucedo opened this issue Mar 5, 2020 · 3 comments · Fixed by #1517
Milestone

Comments

@axsaucedo
Copy link
Contributor

axsaucedo commented Mar 5, 2020

As we see usecases in airgapped clusters, having the registries as a variable that can be changed in a single or fewer places. Potentially having a "global registry prefix" that could be set only if all the global registry is the same, alternatively if the user needs to use multiple registries for any reason this could be set blank.

This would apply primarily to the Seldon Core Analytics and the Seldon Core Operator Helm charts

@axsaucedo axsaucedo added the triage Needs to be triaged and prioritised accordingly label Mar 5, 2020
@axsaucedo axsaucedo added this to the 1.1 milestone Mar 5, 2020
@ukclivecox
Copy link
Contributor

Check if there is a helm chart repo for this.

@ukclivecox ukclivecox removed the triage Needs to be triaged and prioritised accordingly label Mar 6, 2020
@ryandawsonuk
Copy link
Contributor

ryandawsonuk commented Mar 6, 2020

There seem to be two common ways to specify a single image. Either:

image:
  registry:
  repository:
  tag:

Or:

image:
  repository:
  tag:

In the latter the registry is included in the repository entry. See for example https://github.com/helm/charts/blob/master/stable/airflow/values.yaml#L64

If there are other images used by the same chart then they can appear at a different level of indentation - https://github.com/helm/charts/blob/master/stable/airflow/values.yaml#L548

This presents a bit of awkwardness for the user in that they have to override each registry entry individually. It is possible to provide a global override as the mariadb chart does:

https://github.com/helm/charts/blob/283aca3551399d16b1e7b4ba13e1099483f34b04/stable/mariadb/values.yaml#L5

But that does add quite a bit of complexity.

I'd suggest for us that the operator chart is already pretty clear:

https://github.com/SeldonIO/seldon-core/blob/master/helm-charts/seldon-core-operator/values.yaml

And we could make the analytics chart clearer by adding some commented entries in the values file to show how the images are to be set on the charts we embed.

Would also be good to add a note about finding which images to replace (mentioning the references in the operator's configmap) to https://docs.seldon.io/projects/seldon-core/en/latest/graph/private_registries.html or create a new similar page.

@ukclivecox
Copy link
Contributor

But we have the pre-package server images and storage-initializer image using a different format. Should we change those to the more verbose but correct format?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants