-
Notifications
You must be signed in to change notification settings - Fork 26
update helm deploy yamls #62
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 @lanmaoxinqing, 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 focuses on refining and updating the Helm deployment configurations for various services within the application. The changes aim to enhance the flexibility and consistency of deployments by standardizing parameter handling, improving how Object Storage Service (OSS) credentials are managed and distributed, and introducing new configuration options for service behavior. These updates streamline the deployment process and ensure more robust and configurable service operations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 updates several Helm deployment YAML files, introducing new features and refactoring existing configurations. My review focuses on improving the maintainability, consistency, and robustness of the Helm templates. I've identified opportunities to simplify template logic using idiomatic Helm functions, reduce code duplication through helpers, and resolve inconsistencies in how values are handled across different subcharts. There are also a couple of high-severity issues related to missing values and inconsistent variable usage that could cause deployment failures.
| timeoutSeconds: 1 | ||
| env: | ||
| - name: SERVICE_DOMAIN_SUFFIX | ||
| value: {{ .Values.serviceDomainSuffix | quote }} |
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.
The template uses .Values.serviceDomainSuffix, but this value is not defined in deploy/controller/values.yaml. If this value is not provided during Helm installation, template rendering will fail. It's crucial to provide a default value. The best practice is to add serviceDomainSuffix: "" to your values.yaml. As a defensive measure in the template, you can use the default function to prevent errors.
value: {{ .Values.serviceDomainSuffix | default "" | quote }}| kind: Secret | ||
| metadata: | ||
| name: oss-secret | ||
| namespace: {{ .Values.global.sandboxNamespace }} |
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.
There is an inconsistency in how sandboxNamespace is accessed across subcharts. This chart (envhub) uses a global value (.Values.global.sandboxNamespace), while the controller chart uses a local value (.Values.sandboxNamespace). This can lead to configuration errors and makes the umbrella chart difficult to manage. For shared configuration like this, you should standardize on using a single global value defined in the parent chart's values.yaml and accessed via .Values.global.sandboxNamespace in all subcharts.
| - name: api-service | ||
| version: "0.0.1" | ||
| repository: "file://./api-service" | ||
| condition: api-service-k8s.enabled | ||
| alias: api-service-k8s |
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.
Duplicating the api-service dependency introduces a maintenance risk. If the version of the api-service subchart changes, it will need to be updated in two places, which is error-prone. While this is a common Helm pattern for deploying multiple instances of a subchart, it's important to be aware of this potential for inconsistency.
| {{- define "api-service.scheduleType" -}} | ||
| {{ if .Values.scheduleType }} | ||
| {{- .Values.scheduleType -}} | ||
| {{ else }} | ||
| {{- printf "k8s" .Values.metadata.namespace -}} | ||
| {{ end }} | ||
| {{ end }} | ||
|
|
||
| {{- define "api-service.qps" -}} | ||
| {{- if .Values.qps }}{{ printf "%v" .Values.qps }}{{ else }}{{ "10" }}{{ end }} | ||
| {{- 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.
The logic in these new helper templates can be significantly simplified and made more idiomatic. The api-service.scheduleType helper uses printf incorrectly, and both helpers can be made more concise by using the default function.
{{- define "api-service.scheduleType" -}}
{{- .Values.scheduleType | default "k8s" -}}
{{ end }}
{{- define "api-service.qps" -}}
{{- .Values.qps | default "10" -}}
{{- end }}
| {{- if .Values.leaderElect }} | ||
| - --leader-elect=true | ||
| {{- else }} | ||
| - --leader-elect=false | ||
| {{- 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.
| {{if .Values.deploy.templateId}} | ||
| - --template-id={{ .Values.deploy.templateId }} | ||
| {{end}} | ||
| {{if .Values.deploy.callbackURL}} | ||
| - --callback-url={{ .Values.deploy.callbackURL }} | ||
| {{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.
There are a couple of improvements to be made here for style and whitespace control. It's a Helm convention to include a space after if (i.e., {{ if ... }}). Additionally, using {{- and -}} helps control newlines, preventing extra blank lines in the rendered YAML when the conditions are false.
{{- if .Values.deploy.templateId }}
- --template-id={{ .Values.deploy.templateId }}
{{- end }}
{{- if .Values.deploy.callbackURL }}
- --callback-url={{ .Values.deploy.callbackURL }}
{{- end }}| # 为 envhub namespace 创建 OSS secret | ||
| apiVersion: v1 | ||
| kind: Secret | ||
| metadata: | ||
| name: envhub-secret | ||
| name: oss-secret | ||
| namespace: {{ .Values.metadata.namespace }} | ||
| type: Opaque | ||
| data: | ||
| # base64编码的值 | ||
| ACI_ACCESS: "" | ||
| ACI_SECRET: "" | ||
| OSS_ACCESS_KEY_ID: "" | ||
| OSS_ACCESS_KEY_SECRET: "" | ||
| # OSS credentials (base64 encoded) | ||
| {{- if .Values.oss.accessKeyId }} | ||
| oss-ak: {{ .Values.oss.accessKeyId | b64enc | quote }} | ||
| {{- else }} | ||
| oss-ak: "" | ||
| {{- end }} | ||
| {{- if .Values.oss.accessKeySecret }} | ||
| oss-sk: {{ .Values.oss.accessKeySecret | b64enc | quote }} | ||
| {{- else }} | ||
| oss-sk: "" | ||
| {{- end }} | ||
| {{- if .Values.oss.endpoint }} | ||
| oss-endpoint: {{ .Values.oss.endpoint | b64enc | quote }} | ||
| {{- else }} | ||
| oss-endpoint: "" | ||
| {{- end }} | ||
| {{- if .Values.oss.bucket }} | ||
| oss-bucket: {{ .Values.oss.bucket | b64enc | quote }} | ||
| {{- else }} | ||
| oss-bucket: "" | ||
| {{- end }} | ||
| --- | ||
| # 为 aenv-sandbox namespace 创建相同的 OSS secret | ||
| # 用于 service pod 访问 OSS | ||
| apiVersion: v1 | ||
| kind: Secret | ||
| metadata: | ||
| name: oss-secret | ||
| namespace: {{ .Values.global.sandboxNamespace }} | ||
| type: Opaque | ||
| data: | ||
| # OSS credentials (base64 encoded) | ||
| {{- if .Values.oss.accessKeyId }} | ||
| oss-ak: {{ .Values.oss.accessKeyId | b64enc | quote }} | ||
| {{- else }} | ||
| oss-ak: "" | ||
| {{- end }} | ||
| {{- if .Values.oss.accessKeySecret }} | ||
| oss-sk: {{ .Values.oss.accessKeySecret | b64enc | quote }} | ||
| {{- else }} | ||
| oss-sk: "" | ||
| {{- end }} | ||
| {{- if .Values.oss.endpoint }} | ||
| oss-endpoint: {{ .Values.oss.endpoint | b64enc | quote }} | ||
| {{- else }} | ||
| oss-endpoint: "" | ||
| {{- end }} | ||
| {{- if .Values.oss.bucket }} | ||
| oss-bucket: {{ .Values.oss.bucket | b64enc | quote }} | ||
| {{- else }} | ||
| oss-bucket: "" | ||
| {{- 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 file defines the same oss-secret twice, once for each namespace. This leads to significant code duplication and is a maintenance concern. If the secret's structure changes, you'll have to update it in two places.
A better approach is to define a helper template in _helpers.tpl for the secret data and then include it in both secret definitions. This follows the Don't Repeat Yourself (DRY) principle.
For example, in deploy/envhub/templates/_helpers.tpl:
{{- define "envhub.ossSecretData" -}}
# OSS credentials (base64 encoded)
...
{{- end -}}And then in secret.yaml:
...
data:
{{ include "envhub.ossSecretData" . | nindent 2 }}
---
...
data:
{{ include "envhub.ossSecretData" . | nindent 2 }}
JacksonMei
left a comment
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.
lgtm
No description provided.