Skip to content

feat(helm): support global.imageRegistry/global.image.registry#4866

Merged
marcsanmi merged 1 commit intografana:mainfrom
okhowang:main
Mar 18, 2026
Merged

feat(helm): support global.imageRegistry/global.image.registry#4866
marcsanmi merged 1 commit intografana:mainfrom
okhowang:main

Conversation

@okhowang
Copy link
Copy Markdown
Contributor

@okhowang okhowang commented Feb 27, 2026

Note

Low Risk
Low risk: a small Helm templating change that only affects how container image names are constructed when optional registry values are set.

Overview
Adds support for overriding the container image registry in the Pyroscope Helm chart via either a new global.imageRegistry value (applies across the chart) or pyroscope.image.registry (per-chart).

Updates the deployment/statefulset template to prefix images with the chosen registry, and documents/threads the new values through values.yaml and generated jsonnet/values.json.

Written by Cursor Bugbot for commit ef56d98. This will update automatically on new commits. Configure here.

@okhowang okhowang requested a review from a team as a code owner February 27, 2026 08:07
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Feb 27, 2026

CLA assistant check
All committers have signed the CLA.

Copy link
Copy Markdown
Contributor

@simonswine simonswine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite sure if this can/should also impact the sub charts that we have:

alias: agent
version: "0.44.2"
repository: https://grafana.github.io/helm-charts
condition: agent.enabled
- name: alloy
alias: alloy
version: "1.5.2"
repository: https://grafana.github.io/helm-charts
condition: alloy.enabled
- name: minio
alias: minio
version: 4.1.0
repository: https://charts.min.io/
condition: minio.enabled

Note: This would also change the default image to be with docker.io, don't think this is risky.

@marcsanmi
Copy link
Copy Markdown
Contributor

marcsanmi commented Mar 6, 2026

I think this would be a breaking change for users who have a full registry in image.repository today, e.g.:

myregistry.com/grafana/pyroscope:tagdocker.io/myregistry.com/grafana/pyroscope:tag

Could you default image.registry to empty instead of docker.io, and only prepend the registry when explicitly set? Something like:

{{- $registry := .Values.global.imageRegistry | default $values.image.registry -}}
image: "{{ if $registry }}{{ $registry }}/{{ end }}{{ $values.image.repository }}:{{ $values.image.tag | default .Chart.AppVersion }}"

This way existing users with repository: myregistry.com/grafana/pyroscope aren't broken, while the new global.imageRegistry / image.registry feature still works.

@marcsanmi
Copy link
Copy Markdown
Contributor

Thanks for addressing my earlier feedback @okhowang

To address @simonswine's comment about sub-charts: could you rename global.imageRegistryglobal.image.registry? The Grafana sub-charts (alloy, grafana-agent) already read global.image.registry, so Helm will propagate it automatically.

Also, CI needs the generated files updated — please run make helm/docs && make lint and commit the results.

@okhowang
Copy link
Copy Markdown
Contributor Author

I read both global.imageRegistry and global.image.registry now.
because I notice that loki support them both. grafana/loki#19246

@okhowang okhowang changed the title feat(helm): support global.imageRegistry feat(helm): support global.imageRegistry/global.image.registry Mar 18, 2026
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is ON. A cloud agent has been kicked off to fix the reported issue.

@marcsanmi
Copy link
Copy Markdown
Contributor

Thanks for the updates. As you might have noticed, there isn't a consistent standard across Grafana charts for the global image registry key:

  • global.imageRegistry — used by Grafana, Loki, Tempo (single-binary)
  • global.image.registry — used by Alloy, Grafana Agent, Tempo-distributed

Loki ended up supporting both with global.imageRegistry taking precedence and global.image.registry as a deprecated fallback. This PR follows that same approach.

@simonswine — should we support both keys like Loki does (which is what this PR currently implements), or pick just
one? Worth noting that global.image.registry automatically propagates to our alloy/grafana-agent sub-charts since they already read that key, while global.imageRegistry does not.

@marcsanmi
Copy link
Copy Markdown
Contributor

For reference, the Bitnami common library only uses global.imageRegistry and doesn't support global.image.registry.

Given there's no consistent standard across Grafana charts either, maybe I'd lean toward following Bitnami's convention and only supporting global.imageRegistry — simpler, no deprecated fallback to maintain. The trade-off is that it won't
automatically propagate to our sub-charts (alloy/grafana-agent use global.image.registry)

@simonswine
Copy link
Copy Markdown
Contributor

Given there's no consistent standard across Grafana charts either, maybe I'd lean toward following Bitnami's convention and only supporting global.imageRegistry

Agree with that, I have also done a github code search for either and it is about 44k matches for global.imageRegistry vs. 4k global.image.registry.

@marcsanmi
Copy link
Copy Markdown
Contributor

@okhowang I think you need to run make helm/check to make the CI happy.

Copy link
Copy Markdown
Contributor

@marcsanmi marcsanmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for addressing all the changes.

@marcsanmi marcsanmi merged commit 297e06e into grafana:main Mar 18, 2026
26 checks passed
marcsanmi added a commit that referenced this pull request Mar 18, 2026
Bump chart version to include the global.imageRegistry feature from #4866.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@marcsanmi marcsanmi mentioned this pull request Mar 18, 2026
3 tasks
marcsanmi added a commit that referenced this pull request Mar 19, 2026
Bump chart version to include the global.imageRegistry feature from #4866.

Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
aleks-p pushed a commit that referenced this pull request Mar 19, 2026
Bump chart version to include the global.imageRegistry feature from #4866.

Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants