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

Added image registry to radar charts #310

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Added image registry to radar charts #310

wants to merge 4 commits into from

Conversation

keyvaann
Copy link
Collaborator

@keyvaann keyvaann commented Dec 30, 2024

The goal of this PR is to allow image registry to be configured in radar helm charts, which should make it easier to define alternative registries that can act as an pull through cache.

In this process a few things have been changed:

  • Added Bitnami common chart as a dependency to radar charts. This will help with standardizing the charts and using a unified format.
  • To be inline with Bitnami common chart, a backwards incompatible change have been made to use image.pullSecrets instead of imagePullSecrets to define name of the secrets containing image registry credentials.
  • Labels have annotations have an updated templateing format but it shouldn't make any changes to the actual release of the charts.

Copy link

github-actions bot commented Dec 30, 2024

Great PR! Please pay attention to the following items before merging:

Files matching charts/*/values.yaml:

  • Is the PR adding a new container? Please reviewer, add it to the models (internal process)
  • Is the PR adding a new parameter? Please, ensure it’s documented in the README.md

This is an automatically generated QA checklist based on modified files.

@@ -1,8 +1,8 @@
apiVersion: v2
appVersion: "1.0"
appVersion: "RELEASE.2024-11-21T17-21-54Z"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pvannierop I remember we had an issue with s3-proxy application version in the helm chart, I don't remember what exactly it was. Do you think that issue will be present here as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@keyvaann I do not recall this issue to be honest.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was reverted in this commit, 5ac54d6

Copy link
Collaborator

@pvannierop pvannierop left a comment

Choose a reason for hiding this comment

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

I think this a nice change that will:

  1. Allow more extended configuration of the docker image repository (while at the same time clean up the codebase)
  2. Allow for standardization between radarbase helm charts by using a bitnami common chart.

However, I do have a comment/suggestion regarding the standardization. I notice that for all affected charts there is a repeated section added in the _helpers.tpl file such ad this:

{{/*
Return the proper image name
*/}}
{{- define "management-portal.image" -}}
{{ include "common.images.image" (dict "imageRoot" .Values.image "global" .Values.global "chart" .Chart ) }}
{{- end -}}

{{/*
Return the proper Docker Image Registry Secret Names
*/}}
{{- define "management-portal.imagePullSecrets" -}}
{{- include "common.images.pullSecrets" (dict "images" (list .Values.image) "global" .Values.global) -}}
{{- end -}}

Would it not be a good opportunity to define this helper in the common chart? If needed we can wrap the bitnami common chart to add shared RADAR-base functionality?

@keyvaann
Copy link
Collaborator Author

keyvaann commented Jan 3, 2025

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.

2 participants