I don't quite get the added value. In either case the consumer needs to modify .Values.image.repository.
This would only make sense to me if there would a default imageRegistry that would be applied to Values.image.repository and changing the imageRegistry would automatically update Values.image.repository.

I also don't know if that fits into the OCM localization mechanisms.
.Values.image.repository is a de-facto Helm standard. Therefore I would assume that a OCM localization rule for Helm can already work with that and that there is no such change in the chart needed. Otherwise every open source helm chart would need to be modified in order to work with OCM localization.

Maybe @fabianburth can share more details on the OCM localization.

Hi @reshnm

You are correct about the Helm standard and .Values.image.repository. That is why the change does not affect this part or in other words people will still be able to define the registry/repository-path to their image using .Values.image.repository. The idea for this registry value is related to the OCM localization rules. During localization the OCM controller parses the fully qualified path of provider image (e.g. ghcr.io/openmcp-project/github.com/openmcp-project/metrics-operator/images/metrics-operator:v0.6.1

The parsing of the above image is done you get four parts:

> registry: ghcr.io
> repository: openmcp-project/github.com/openmcp-project/metrics-operator/images/metrics-operator 
> image: ghcr.io/openmcp-project/github.com/openmcp-project/metrics-operator/images/metrics-operator:v0.6.1
> tag: v0.6.1

These values then could be injected during runtime in the corresponding paths in values.yaml file. The issue is that if we have only defined .Values.image.repository, we cannot actually substitute anything with the above four values in order to build it.

localization:
- file: values.yaml
  registry: global.imageRegistry # substitude registry portion
  resource:
    name: image
- file: values.yaml # target file for substitution
  repository: image.repository # substitude repository portion
  resource:
    name: image

The above snippet shows that for the registry portion of our example fully qualified image path, we get that value and substitute in in the corresponding path. The same is done for other rules.

Based on our 4 parsed values (registry, repository, image and tag) we cannot substitute anywhere since the values won't be properly interpenetrated and we get gibberish.

The global.imageRegistry is not required to be present in default values.yaml file, so users won't be even aware that this exists. Perhaps only people are in need of something that I am describing as a use case.

Cheers,
Jordan

@jpraychev I get what you are trying to do. But again, it seems unreasonable to me that a Helm chart, in order to work with OCM localization, has to be modified when it uses Helm de-facto standards (means created by helm init).
Imagine someone would wan't to deploy Istio via helm chart. This would mean to either write and maintain your own helm chart or try to change the upstream helm chart.
I can't imagine that this is what we want to do.

On a additional note. I would assume that you would want to localize the complete image location and not only the base path.
I would also assume that the OCI image location of a OCM resource would be available during localization.

Not an OCM expert but I would assume that if you like a concrete version to be deployed you would need to specify the tag as well. That is something already supported by OCM and helm values provide such value. If the tag is not specified, there is a default behavior and falls back to latest tag, which might be desired in some case or not in other - depends on the use case.

The same logic is already present in other projects such as open telemetry collectors - https://github.com/open-telemetry/opentelemetry-helm-charts/blob/4106860b16146f4a696f79b8ba196c8cf683c0ae/charts/opentelemetry-collector/templates/_pod.tpl#L37-L41

Would have been nice for the OCM controller to output directly the registry+repository portion but that is not the case at the moment. Maybe in the future release they could do it, really can't tell.

Would have been nice for the OCM controller to output directly the registry+repository portion but that is not the case at the moment. Maybe in the future release they could do it, really can't tell.

No, you don't want the registry/repository portion. You want to localize the value of the OCM image resource access.
This musn't necessarely match with the repository context of the component. This approach does only work for a very specific use case where you just need to swap the repository context base path. For all other cases, this change won't work.

What I meant is that if OCM have produced a value for registry+repository (e.g. ghcr.io/openmcp-project/github.com/openmcp-project/metrics-operator/images/metrics-operator) you should be able to substitute the value directly into .Values.image.repository and then use that value into template files. That way you won't be needing to have a separate values for registry/repository as in this PR.

What I meant is that if OCM have produced a value for registry+repository (e.g. crimson-prod.common.repositories.cloud.sap/openmcp-project/github.com/openmcp-project/metrics-operator/images/metrics-operator) you should be able to substitute the value directly into .Values.image.repository and then use that value into template files. That way you won't be needing to have a separate values for registry/repository as in this PR.

This is what you always want to do. You should always read the the .Values.image.repository out of the OCM resource. This is the source of truth. You should not contruct it in any other way.

Correct, but OCM does not support it. You can only read the following four values which does not "fit" into the current values.yaml structure for some project.

> registry: ghcr.io
> repository: openmcp-project/github.com/openmcp-project/metrics-operator/images/metrics-operator 
> image: ghcr.io/openmcp-project/github.com/openmcp-project/metrics-operator/images/metrics-operator:v0.6.1
> tag: v0.6.1

Another example that we've got is Dynatrace operator. They don't expose separate registry and repository values but they've got the image value in their values.yaml which maps perfectly to the image portion of OCM.

Can't you then compine <registry>+/+<repository> in the localization?

If not, then I would advice to provide the feedback to OCM that the localization is not working with helm charts created with helm init (which is like 90% of all helm charts)

I'm not happy with changing the helm chart because there are limitations in the OCM localization.

I cannot directly combine it, I would need some custom logic to parse the path myself.

Again, to wrap this up. For me it doesn't make sense to merge this pull request. I see that this would be the easy fix for you to progress. But this not the right approach. OCM localization has to work with "standard" helm charts and not the other way around. Therefore I would like to ask to take this back to the OCM project and get localization solution that works out of the box for "standard" helm charts.

Chaning Helm charts is not the correct approach, as again, you will not find open ears in other open source projects to make their chart compatible with OCM localization. Your pull request would be closed right away. Therefore, again, this has to be taken care of in OCM localization.

Thanks for the feedback @reshnm. Closing the PR.