Skip to content

add imagePullSecrets in helm chart #500

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.

Sign up for GitHub

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

Open
lx1036 wants to merge 1 commit into
base: main
Choose a base branch
from
+9 −0
Open

add imagePullSecrets in helm chart #500

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions charts/kthena/charts/networking/templates/kthena-router/component/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,3 +137,7 @@ spec:
optional: true
{{- end }}
serviceAccountName: kthena-router
{{- with .Values.global.imagePullSecrets }}
imagePullSecrets:
{{- toYaml . | nindent 8 }}
{{- end }}
Comment on lines +140 to +143
Copy link

@gemini-code-assist gemini-code-assist bot Nov 20, 2025

Choose a reason for hiding this comment

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

high

While adding imagePullSecrets directly to the Deployment works, a more robust and maintainable approach in Helm charts is to associate them with the ServiceAccount. This ensures that any Pod using this ServiceAccount automatically gets the imagePullSecrets, reducing duplication and potential for errors if new workloads are added.

I recommend removing this logic from the Deployment and adding it to the kthena-router ServiceAccount template instead.

Here's an example of how you could modify the ServiceAccount template:

apiVersion: v1
kind: ServiceAccount
metadata:
  name: kthena-router
  # ... other metadata
{{- with .Values.global.imagePullSecrets }}
imagePullSecrets:
  {{- toYaml . | nindent 2 }}
{{- end }}

4 changes: 4 additions & 0 deletions charts/kthena/charts/workload/templates/kthena-controller-manager/component/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,7 @@ spec:
secretName: {{ .Values.controllerManager.webhook.tls.certSecretName }}
optional: true
serviceAccountName: kthena-controller-manager
{{- with .Values.global.imagePullSecrets }}
imagePullSecrets:
{{- toYaml . | nindent 8 }}
{{- end }}
Comment on lines +62 to +65
Copy link

@gemini-code-assist gemini-code-assist bot Nov 20, 2025

Choose a reason for hiding this comment

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

high

Similar to the feedback on the kthena-router deployment, it's better to associate imagePullSecrets with the kthena-controller-manager ServiceAccount. This centralizes the configuration and improves chart maintainability. Please remove this block and update the corresponding ServiceAccount template.

1 change: 1 addition & 0 deletions charts/kthena/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,3 +72,4 @@ global:
# This is ONLY required when certManagementMode is set to "manual".
# You can generate it with: cat /path/to/your/ca.crt | base64 | tr -d '\n'
caBundle: ""
imagePullSecrets: []
Copy link

@gemini-code-assist gemini-code-assist bot Nov 20, 2025

Choose a reason for hiding this comment

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

high

A schema definition for this new global.imagePullSecrets value is missing in charts/kthena/values.schema.json. Adding it is important for validating user-provided values and preventing misconfigurations. Please add the corresponding schema definition.

Example for values.schema.json:

...
"global": {
  "type": "object",
  "properties": {
    // ... existing properties
    "imagePullSecrets": {
      "type": "array",
      "description": "Secrets for pulling images from a private registry.",
      "items": {
        "type": "object",
        "properties": {
          "name": {
            "type": "string",
            "description": "The name of the image pull secret."
          }
        },
        "required": ["name"]
      }
    }
  }
}
...

Copy link
Member

@hzxuzhonghu hzxuzhonghu Nov 20, 2025

Choose a reason for hiding this comment

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

It is not clear how to sepcify secrets Can you add a comment

Copy link
Member

@hzxuzhonghu hzxuzhonghu Dec 1, 2025

Choose a reason for hiding this comment

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

@lx1036 would you please update

Loading