fix: allow user to override image repository

[ryanfaircloth]

Allow the image repo to be set in values so that customers can use private registries. Also set pullPolicy in values to the default rather than null

[scholzj]

Thanks for the PR. The idea loosk good. But you need to do the changes to the /packaging/helm-charts directory. The /helm-charts directory is changed only by a new release.

[ryanfaircloth]

Thanks for the PR. The idea loosk good. But you need to do the changes to the /packaging/helm-charts directory. The /helm-charts directory is changed only by a new release.

Oh, didn't catch that the first time amended to change the correct files only

[scholzj]

Ok, look like we have one more problem. The reguar installation files are generated from this and you changed them by adding the image pull policy. You would either need to commit those changes to the PR or maybe better, keep the field unset and set it in tzhe deployment only if set by the user. E.g. something like this in the operators Helm chart: https://github.com/strimzi/strimzi-kafka-operator/blob/main/helm-charts/helm3/strimzi-kafka-operator/templates/060-Deployment-strimzi-cluster-operator.yaml#L65-L67

Sorry, I should have noticed it the first time.

[ryanfaircloth]

Ok, look like we have one more problem. The reguar installation files are generated from this and you changed them by adding the image pull policy. You would either need to commit those changes to the PR or maybe better, keep the field unset and set it in tzhe deployment only if set by the user. E.g. something like this in the operators Helm chart: https://github.com/strimzi/strimzi-kafka-operator/blob/main/helm-charts/helm3/strimzi-kafka-operator/templates/060-Deployment-strimzi-cluster-operator.yaml#L65-L67

Sorry, I should have noticed it the first time.

I removed the pullPolicy when I reviewed the code again I realized it wasn't needed I also accidentally changed the default tag in this path from latest to a version which wasn't needed becasue your setting that when you build the chart.

[scholzj]

@im-konge How are the Helm Chart STs here supposed to work? From the code, it seems they expect here to be three values in the Helm Chart:

• image.registry
• image.repository
• image.tag

And there likely should be fourth part image.name for the name of the image which the STs do not expect to change.

But it seems that only the image.tag actually worked until now. So the tests just worked by using the previous image I guess until now? And not sure why they stopped working now. Can you please look at it?

I guess we should follow what we do in other Strimzi Helm Charts and have:

image:
  registry: quay.io
  repository: strimzi
  name: access-operator
  tag: latest
  pullPolicy: ""

And have something like this in the Deployment?

          image: {{ .Values.image.registry }}/{{ .Values.image.repository}}/{{ .Values.image.name }}:{{ .Values.image.tag }}
          {{- if .Values.image.pullPolicy }}
          imagePullPolicy: {{ .Values.image.pullPolicy }}
          {{- end }}

Do you think that would make the STs work?

[im-konge]

@scholzj I will have a look today at the morning. Sorry for the issues.

[im-konge]

@scholzj yeah I think that when I was writing the tests and everything, I just copied the stuff we have for DrainCleaner for the Helm installation -> there we have exactly this:

          image: {{ .Values.image.registry }}/{{ .Values.image.repository}}/{{ .Values.image.name }}:{{ .Values.image.tag }}
          {{- if .Values.image.pullPolicy }}
          imagePullPolicy: {{ .Values.image.pullPolicy }}
          {{- end }}

so having the whole block as you mentioned should fix the issue. For the name -> I can create a PR here and also to the DrainCleaner repo to make it configurable. The reason why I didn't add it there is that we actually never needed to change the name of the image and kept it as it is (even when we are building the images in the AZP).

[im-konge]

@ryanfaircloth could you please change the values.yaml and the Deployment YAML as @scholzj suggested? It should fix the issue with the tests and also make it consistent to the Deployment files we have in Strimzi and DrainCleaner.

Because currently, we are configuring the registry to point to local ones, but it should be just instead of quay.io -> the way you have it now, it will be changed completely and it will not match -> the image will fail to pull and the whole installation will fail.

Thanks a lot! And sorry for issues... I should have check it sooner.

[ryanfaircloth]

No problem changes made. I also found a better solution to the syntax highly error with pullPolicy

[im-konge]

LGTM, thanks :)

[scholzj]

Thanks @ryanfaircloth