add imagePullSecrets in helm chart #500
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello @lx1036, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Helm chart for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Secrets to pull images, Helm chart now knows the way, Pods start up with ease. Footnotes
|
|
Welcome @lx1036! It looks like this is your first PR to volcano-sh/kthena 🎉 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for imagePullSecrets, which is a great enhancement. My review provides feedback to improve the implementation based on Helm chart best practices. The main suggestions are to associate imagePullSecrets with ServiceAccounts for better maintainability, and to add the corresponding schema definition in values.schema.json for robust validation. These changes will make the chart more robust and easier to use.
| {{- with .Values.global.imagePullSecrets }} | ||
| imagePullSecrets: | ||
| {{- toYaml . | nindent 8 }} | ||
| {{- end }} |
There was a problem hiding this comment.
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 }}| {{- with .Values.global.imagePullSecrets }} | ||
| imagePullSecrets: | ||
| {{- toYaml . | nindent 8 }} | ||
| {{- end }} |
There was a problem hiding this comment.
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.
| # 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: [] |
There was a problem hiding this comment.
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"]
}
}
}
}
...
| # 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: [] |
There was a problem hiding this comment.
It is not clear how to sepcify secrets Can you add a comment
Signed-off-by: lx1036 <[email protected]>
hzxuzhonghu
force-pushed
the
feature/add-imagepullsecrets
branch
from
e1acd4b to
7bc5fd8
Compare
3 participants
What type of PR is this?
/kind enhancement
What this PR does / why we need it:
Add imagePullSecrets in helm chart, so we can helm install like:
helm upgrade --install kthena .. --namespace kthena-system --create-namespace --version 1.0.0 --set global.imagePullSecrets[0].name=docker-login