-
Notifications
You must be signed in to change notification settings - Fork 3.9k
fix(helm): standardize global image registry to match other Grafana charts #19246
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
fix(helm): standardize global image registry to match other Grafana charts #19246
Conversation
production/helm/loki/Chart.yaml
Outdated
| type: application | ||
| appVersion: 3.5.3 | ||
| version: 6.40.0 | ||
| version: 6.40.1 |
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.
You don't need to update the version number, this is handled automatically as part of the automation. See here for an example.
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.
@JStickler Thank you for pointing that out! I understand now that chart version updates are handled automatically by the release automation. I've reverted the version change from 6.40.1 back to 6.40.0.
Thanks to your guidance, I now have a better understanding of the project's workflow and won't repeat these mistakes. Thank you for your patience! 🫡
Fixes grafana#19212 This PR standardizes the global image registry configuration to match other Grafana charts by supporting the standard global.imageRegistry format while maintaining backwards compatibility. Changes: - Add support for standard global.imageRegistry in _helpers.tpl - Add global.imageRegistry to values.yaml with proper documentation - Maintain backwards compatibility with existing global.image.registry - Update precedence order: global.imageRegistry > global.image.registry > global.registry Users can now use the standard format across all Grafana charts: global: imageRegistry: my-registry.com While existing configurations continue to work: global: image: registry: my-registry.com Signed-off-by: puretension <[email protected]>
5a80da6 to
89595ea
Compare
jkroepke
left a comment
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.
Make sense, since grafana helm follow the same
| */}} | ||
| {{- define "loki.baseImage" }} | ||
| {{- $registry := .global.registry | default .service.registry | default "" -}} | ||
| {{- $registry := .global.imageRegistry | default ((.global.image).registry) | default .global.registry | default .service.registry | default "" -}} |
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.
What about non loki image, e.g nginx and memcache?
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.
@jkroepke Thank you for catching this critical issue! You're absolutely right and this was the hidden root cause I missed.
While I updated the loki.baseImage helper to support global.imageRegistry, I only applied it to the main Loki image. The nginx, memcached, memcachedExporter, and sidecar images were still using direct
{{ .repository }}:{{ .tag }} format, completely bypassing the global registry logic.
I've now extended the fix to ensure all images consistently use the loki.baseImage helper:
- Memcached & MemcachedExporter images
- Sidecar images (single-binary, backend, ruler deployments)
- Gateway (nginx) was already using
loki.gatewayImage→loki.baseImage
This ensures true global registry standardization across the entire chart, matching the Grafana helm pattern you referenced.
Example usage:
yaml
global:
imageRegistry: my-registry.com
Great catch on the incomplete implementation! 🙏
- Extended global.imageRegistry support to memcached, memcachedExporter, and sidecar images - All images now consistently use loki.baseImage helper for global registry override - Ensures complete standardization across the entire chart Addresses reviewer feedback on incomplete image coverage. Signed-off-by: puretension <[email protected]>
|
@puretension could you add an |
|
@puretension Still missing a CHANGELOG entry. Please add one. |
…tion Add CHANGELOG entry for PR grafana#19246 that standardizes global image registry configuration to match other Grafana charts. Signed-off-by: puretension <[email protected]>
|
@jkroepke Both requests have been addressed! |
|
This should be merged before #19347 gets merged. LGTM |
Jayclifford345
left a comment
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.
LGTM
Since grafana#19246 the sidecar image would resolve to registry-mirror.example.com/docker.io/kiwigrid/k8s-sidecar when a global registry mirror was defined. Signed-off-by: Sefa Eyeoglu <[email protected]>
Since grafana#19246 the sidecar image would resolve to registry-mirror.example.com/docker.io/kiwigrid/k8s-sidecar when a global registry mirror was defined. Signed-off-by: Sefa Eyeoglu <[email protected]>
Since grafana#19246 the sidecar image would resolve to registry-mirror.example.com/docker.io/kiwigrid/k8s-sidecar when a global registry mirror was defined. Signed-off-by: Sefa Eyeoglu <[email protected]>
Since grafana#19246 the sidecar image would resolve to registry-mirror.example.com/docker.io/kiwigrid/k8s-sidecar when a global registry mirror was defined. Signed-off-by: Sefa Eyeoglu <[email protected]>
Since grafana#19246 the sidecar image would resolve to registry-mirror.example.com/docker.io/kiwigrid/k8s-sidecar when a global registry mirror was defined. Signed-off-by: Sefa Eyeoglu <[email protected]>
Since grafana#19246 the sidecar image would resolve to registry-mirror.example.com/docker.io/kiwigrid/k8s-sidecar when a global registry mirror was defined. Signed-off-by: Sefa Eyeoglu <[email protected]>
Since grafana#19246 the sidecar image would resolve to registry-mirror.example.com/docker.io/kiwigrid/k8s-sidecar when a global registry mirror was defined. Signed-off-by: Sefa Eyeoglu <[email protected]>
|
This introduced some problems mentioned in #19662 |
What this PR does / why we need it:
This PR standardizes the global image registry configuration to match other Grafana charts by supporting the standard
global.imageRegistryformat while maintaining backwards compatibility with the existingglobal.image.registryformat.Currently, Loki chart uses a different structure for global image registry override compared to other Grafana charts:
global.image.registryglobal.imageRegistryThis inconsistency forces users to configure different charts differently, contradicting the purpose of having a global override.
Which issue(s) this PR fixes:
Fixes #16511
Fixes #19212
Special notes for your reviewer:
This change maintains full backwards compatibility - existing configurations using
global.image.registrywill continue to work. The precedence order is:global.imageRegistry>global.image.registry>global.registry>service.registry.The existing
global.image.registryis marked as deprecated in documentation to encourage migration to the standard format.Checklist
CONTRIBUTING.mdguide (required)docs/sources/setup/upgrade/_index.md(not required - backwards compatible)deprecated-config.yamlanddeleted-config.yamlfiles respectively (not applicable - no removal, only deprecation notice in comments)Changes Made
_helpers.tpl: Modifiedloki.baseImagetemplate to support standardglobal.imageRegistryvalues.yaml: Addedglobal.imageRegistrywith proper documentation and marked existing format as deprecatedBefore/After
Before:
After (both work, standard recommended):