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

feat(charts): add global.imageRegistry parameter #227

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

LKummer
Copy link

@LKummer LKummer commented Dec 25, 2024

Pull Request template

Why is this PR required? What issue does it fix?:

Fixes #203.

What this PR does?:

  • Adds global.imageRegistry Helm parameter for specifying an image registry to be used for all images. It is overridden by localpv.image.registry and helperPod.image.registry when specified.
  • Adds support for specifying image registries without a trailing /, while still supporting a trailing / to avoid breaking existing configurations.

Does this PR require any upgrade changes?:

No.

If the changes in this PR are manually verified, list down the scenarios covered::

  • Installation with default values.
  • Installation with global.imageRegistry set without trailing /.
  • Installation with global.imageRegistry and localpv.image.registry set without trailing /.
  • Installation with global.imageRegistry and helperPod.image.registry set without trailing /.
  • Installation with global.imageRegistry, localpv.image.registry and helperPod.image.registry set without trailing /.
  • Installation with localpv.image.registry set with trailing /.
  • Installation with helperPod.image.registry set with trailing /.

Any additional information for your reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs

No.

Checklist:

@LKummer
Copy link
Author

LKummer commented Dec 26, 2024

I tried to force push to re-run the CI job, it seems to have failed before installing the chart in the integration test.

@tiagolobocastro
Copy link

I suspect we need something like this: https://github.com/openebs/lvm-localpv/pull/357/files

@tiagolobocastro
Copy link

CC @niladrih

@niladrih
Copy link
Member

niladrih commented Jan 9, 2025

CI check fails due to CI issue. Fix: #229.

@niladrih
Copy link
Member

@LKummer could you please rebase your branch? The CI should run better after the recent changes.

Add global.imageRegistry option overridden by localpv.image.registry and
helperPod.image.registry for localpv and helper Pod respectively.
Fix registry options requiring a trailing slash while still supporting a
trailing slash to avoid breaking existing configurations.

Signed-off-by: Lior Friedman <[email protected]>
@LKummer
Copy link
Author

LKummer commented Jan 10, 2025

Thank you @niladrih, I've rebased my branch.

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 37.35%. Comparing base (cab53c4) to head (0657f34).
Report is 14 commits behind head on develop.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #227      +/-   ##
===========================================
- Coverage    37.91%   37.35%   -0.57%     
===========================================
  Files           36        1      -35     
  Lines         3373      779    -2594     
===========================================
- Hits          1279      291     -988     
+ Misses        2012      479    -1533     
+ Partials        82        9      -73     
Flag Coverage Δ
integrationtests 37.35% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@tiagolobocastro tiagolobocastro left a comment

Choose a reason for hiding this comment

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

This would be good to have on all openebs charts

@@ -45,7 +45,7 @@ spec:
{{- toYaml .Values.podSecurityContext | nindent 8 }}
containers:
- name: {{ template "localpv.fullname" . }}
image: "{{ .Values.localpv.image.registry }}{{ .Values.localpv.image.repository }}:{{ .Values.localpv.image.tag }}"
image: "{{ with .Values.localpv.image.registry | default .Values.global.imageRegistry | trimSuffix "/" }}{{ . }}/{{ end }}{{ .Values.localpv.image.repository }}:{{ .Values.localpv.image.tag }}"

Choose a reason for hiding this comment

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

nit: a function in helpers might be good to have here

@niladrih
Copy link
Member

Thanks for this change @LKummer. I'll merge when you resolve the review conversation (as in click on the 'Resolve conversation' button, up to you if you'll take the suggestion or not).

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.

Helm chart should read image registry name from global.imageRegistry variable
4 participants