Feat/helm chat service#121
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @NotYuSheng, 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 significantly enhances the deployment and operational capabilities for the chat-service by introducing a dedicated Helm chart. This chart, coupled with a new Makefile, streamlines the process of deploying, managing, and interacting with the service within a Kubernetes environment, making it easier for developers to get started and maintain the application.
Highlights
- New Helm Chart: Introduced a new Helm chart for the
chat-service, enabling its standardized deployment and management on Kubernetes. - Makefile for Helm Operations: Added a comprehensive
Makefileto simplify common Helm and Kubernetes operations, including single-service and multi-service commands for installation, upgrades, uninstallation, linting, status checks, and port-forwarding. - Kubernetes Resource Definitions: The new Helm chart includes templates for Kubernetes Deployment, Service, Ingress, Horizontal Pod Autoscaler (HPA), ServiceAccount, and connection tests, all configurable via
values.yaml. - Configurable Deployment Parameters: Provided a
values.yamlfile with default and configurable parameters for thechat-servicedeployment, such as image details, replica count, service type, ingress settings, resource limits, and environment variables.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a Helm chart for the chat-service and a Makefile to streamline Helm operations. The review focuses on improving the correctness, security, and robustness of the new Helm chart and Makefile. Key findings include a critical issue where environment variables are not passed to the deployment, security concerns with hardcoded secrets, and several medium-severity issues related to configuration and scripting practices.
| {{- toYaml .Values.readinessProbe | nindent 12 }} | ||
| resources: | ||
| {{- toYaml .Values.resources | nindent 12 }} | ||
| {{- with .Values.volumeMounts }} |
There was a problem hiding this comment.
The env variables defined in values.yaml are not being applied to the container. This will likely cause the application to fail at runtime as it seems to depend on them. You should add the env block to the container spec to pass these variables to the application.
{{- with .Values.env }}
env:
{{- toYaml . | nindent 12 }}
{{- end }}
| - name: OPENAI_BASE_URL | ||
| value: http://vllm-qwen:1234/v1 | ||
| - name: OPENAI_API_KEY | ||
| value: lm-studio | ||
| - name: OPENAI_MODEL | ||
| value: qwen2.5-0.5b-instruct |
There was a problem hiding this comment.
Hardcoding secrets like API keys in values.yaml is a security risk, as they can be accidentally committed to version control. It is highly recommended to use Kubernetes Secrets to store sensitive data and reference them in the pod specification.
|
|
||
| 1. Access your application via port-forward: | ||
| export POD_NAME=$(kubectl get pods --namespace {{ .Release.Namespace }} -l "app.kubernetes.io/name={{ include "chat-service.name" . }},app.kubernetes.io/instance={{ .Release.Name }}" -o jsonpath="{.items[0].metadata.name}") | ||
| kubectl --namespace {{ .Release.Namespace }} port-forward $$POD_NAME 8080:8080 |
There was a problem hiding this comment.
The hardcoded target port 8080 in the port-forward command is incorrect. The service is running on port {{ .Values.service.port }} (which is 8000 according to values.yaml). This command will fail or connect to the wrong port. You should use the value from values.yaml to ensure it's always correct.
kubectl --namespace {{ .Release.Namespace }} port-forward $$POD_NAME 8000:{{ .Values.service.port }}
|
|
||
| # Default port for port-forwarding (override as needed) | ||
| LOCAL_PORT ?= 8080 | ||
| REMOTE_PORT ?= 8080 |
There was a problem hiding this comment.
The default REMOTE_PORT is 8080, but the chat-service introduced in this PR runs on and exposes port 8000. This default will not work for the chat-service without being overridden. Consider changing the default to match the service's port.
REMOTE_PORT ?= 8000
| kubectl --namespace $(NAMESPACE) port-forward \ | ||
| $$(kubectl get pod -n $(NAMESPACE) -l "app.kubernetes.io/name=$(CHART_NAME),app.kubernetes.io/instance=$(CHART_NAME)" -o jsonpath="{.items[0].metadata.name}") \ | ||
| $(LOCAL_PORT):$(REMOTE_PORT) |
There was a problem hiding this comment.
The current port-forward implementation targets a specific pod by name. This is fragile because pods can be rescheduled, and it doesn't work well with multiple replicas (it will always pick the first one). It's more robust to port-forward to the service instead. This also simplifies the command.
kubectl --namespace $(NAMESPACE) port-forward service/$(CHART_NAME) $(LOCAL_PORT):$(REMOTE_PORT)
| # incremented each time you make changes to the application. Versions are not expected to | ||
| # follow Semantic Versioning. They should reflect the version the application is using. | ||
| # It is recommended to use it with quotes. | ||
| appVersion: "1.16.0" |
There was a problem hiding this comment.
The appVersion is set to "1.16.0", which seems arbitrary and doesn't align with the image.tag ("dev-v0.0.0-e8b9d89") in values.yaml. The appVersion should reflect the version of the application being deployed. It's recommended to keep this version in sync with your application's versioning scheme for clarity and better release management.
| # This is to setup the liveness and readiness probes more information can be found here: https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes/ | ||
| livenessProbe: | ||
| httpGet: | ||
| path: / |
There was a problem hiding this comment.
The liveness probe is configured to target the root path (/). While this might work, it's better practice to use a dedicated health check endpoint. The application seems to have a /health router, so a /health endpoint is likely available. Using a specific health endpoint is more reliable and avoids exercising the main application logic for health checks.
path: /health
| port: http | ||
| readinessProbe: | ||
| httpGet: | ||
| path: / |
There was a problem hiding this comment.
Similar to the livenessProbe, the readinessProbe should also target a dedicated health endpoint like /health for better reliability and to ensure the app is truly ready to serve traffic.
path: /health
1 participant
Summary
Fixes #<issue_number>
Changes Made
Context / Rationale
Related Docs or References
FastAPI Application Checklist (Delete if PR is not relevant)
/healthendpoint is implemented and returns 200 OK/docsor/redocfeature/*,bugfix/*) — do not usedevdirectlyGeneral Checklist