-
Notifications
You must be signed in to change notification settings - Fork 2
feat: add serviceaccount in common and onechart #3
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
Conversation
Summary of ChangesHello @bankhubjar, 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 introduces a standardized and reusable mechanism for defining Kubernetes ServiceAccounts across Helm charts. By creating a common ServiceAccount template and integrating it into 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. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds a ServiceAccount resource to the onechart Helm chart by creating a reusable template in the common library chart. The implementation has a critical bug due to a case-sensitivity issue that will prevent it from working correctly. I've provided a suggestion to fix this bug and refactor the implementation to follow common Helm chart best practices, making it more flexible and robust by using a structured configuration value. This change also includes adding standard labels and using a generated name by default, which are recommended practices.
| {{- if .Values.serviceaccount }} | ||
| --- | ||
| apiVersion: v1 | ||
| kind: ServiceAccount | ||
| metadata: | ||
| name: {{ .Values.serviceAccount }} | ||
| namespace: {{ .Release.Namespace }} | ||
| {{- end }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation has a critical bug and could be made more flexible and robust.
- Bug: There is a case mismatch between
.Values.serviceaccountin theifcondition and.Values.serviceAccountused for the name. This will prevent the template from working as intended. - Inflexible Design: Using a single value to both enable creation and provide the name is not ideal. For example, if a user sets
serviceAccount: true, the resource name would be invalid. A structured object is the standard practice in Helm charts.
I recommend refactoring this to use a structured value. This also makes it easy to add labels and use a default name for the service account, improving consistency with other resources. This would require adding the following to onechart/values.yaml:
serviceAccount:
create: false
# name: my-service-accountNote: The suggestion below uses onechart.fullname and onechart.labels. Since this template is in a library chart, hardcoding the parent chart name (onechart) is not ideal for reusability. A more advanced approach would be needed for a truly generic template, but this is a good improvement for the current use case.
{{- if .Values.serviceAccount.create -}}
---
apiVersion: v1
kind: ServiceAccount
metadata:
name: {{ .Values.serviceAccount.name | default (include "onechart.fullname" .) }}
namespace: {{ .Release.Namespace }}
labels:
{{- include "onechart.labels" . | nindent 4 }}
{{- end }}
No description provided.