Feature/helm setup#108
Conversation
NotYuSheng
added this to the
Sprint 4 – Pre-Staging Onboarding & Integration (Week 7–8) milestone
![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 our deployment capabilities by integrating Helm for Kubernetes-native application management. It provides a dedicated Helm chart for the chat-service and introduces a centralized Makefile to streamline common deployment tasks, ensuring a more consistent and efficient approach to managing our microservices in a Kubernetes environment.
Highlights
- Helm Chart for Chat Service: I've introduced a new Helm chart specifically for the
chat-servicemicroservice, located underhelm/chat-service/. This chart includes standard Kubernetes manifests for Deployment, Service, Horizontal Pod Autoscaler (HPA), Ingress, and ServiceAccount, all configurable via a comprehensivevalues.yamlfile. It also includes aNOTES.txtfor post-installation guidance and a basic test connection pod. - Centralized Makefile for Deployment Management: A new
Makefilehas been added to the root directory to centralize and simplify Helm and Kubernetes operations. It provides targets for installing, upgrading, uninstalling, linting, and checking the status of individual Helm charts, as well as batch operations (install-all,upgrade-all,uninstall-all) for all charts under thehelm/directory. Aport-forwardtarget is also included for local access. - Standardized Deployment Scaffolding: This PR establishes a foundational structure for production-ready deployments across OmniPDF microservices. By leveraging Helm and a unified
Makefile, it aims to enforce standardization, simplify deployment workflows, and prepare the project for future CI/CD integration.
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 is currently in preview and 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 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
The pull request introduces Helm chart support for the chat-service microservice and a centralized Makefile for managing deployments. I've provided feedback to improve the robustness, security, and portability of the new deployment scripts and Helm chart. The most critical issues relate to a hardcoded API key and missing environment variables in the deployment.
|
|
||
| 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 port-forward command in the notes has a hardcoded container port (8080). However, the actual container port is configurable via .Values.service.port (which defaults to 8000).
This instruction will fail or connect to the wrong port if the service port is changed. The command should use the value from values.yaml to provide accurate instructions to the user.
kubectl --namespace {{ .Release.Namespace }} port-forward $$POD_NAME 8080:{{ .Values.service.port }}
|
Add helm linter issue |
…loyment automation Introduces Helm chart support for the chat-service microservice and a centralized Makefile for managing deployments across all services, with comprehensive production hardening. Address #98 Changes Made: - Created Helm chart for chat-service under helm/chat-service/ - Defined Kubernetes manifests for Deployment, Service, and Helm NOTES.txt - Introduced values.yaml with configurable ports, replica count, image name, and probes - Added centralized Makefile with install, upgrade, uninstall, status, and lint for individual charts - Included install-all, upgrade-all, uninstall-all for batch operations - Added configurable port-forward target - Included examples and usage notes via make help - Added production-ready resource limits (200m CPU, 256Mi memory) - Configured security hardening (non-root user, read-only filesystem, dropped capabilities) - Fixed environment variable injection in deployment template - Enabled autoscaling with realistic limits (2-10 replicas) - Added comprehensive health checks (startup, liveness, readiness probes) - Added local image repository setup documentation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Use helm upgrade --install for robust deployment in Makefile - Fix image repository name (chat_service -> chat-service) following K8s conventions - Remove hardcoded image tag, use Chart.yaml appVersion instead - Fix hardcoded ports in NOTES.txt to use dynamic service port values - Set pullPolicy to Never for offline production environments - Add missing startupProbe to deployment template - Improve chart portability and offline deployment readiness 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This PR introduces Helm chart support for the chat-service and a Makefile for deployment management. The changes are well-structured and follow Helm best practices. My review focuses on improving security by removing hardcoded secrets, increasing robustness of Makefile targets and Helm templates, and ensuring consistency in naming and documentation.
| helm install $$CHART helm/$$CHART \ | ||
| --namespace $(NAMESPACE) \ | ||
| --create-namespace \ | ||
| --values helm/$$CHART/values.yaml; \ |
There was a problem hiding this comment.
The install-all target uses helm install, which will fail if a release with the same name already exists. For idempotency and consistency with the single install target (which uses helm upgrade --install), it's better to use helm upgrade --install here as well. This ensures the command can be run multiple times safely, either installing or upgrading the charts.
helm upgrade --install $$CHART helm/$$CHART \
--namespace $(NAMESPACE) \
--create-namespace \
--values helm/$$CHART/values.yaml; \
|
|
||
| # Update deployment with new image | ||
| make upgrade CHART_NAME=chat-service | ||
| ``` No newline at end of file |
There was a problem hiding this comment.
This file is missing a final newline. It's a good practice to end all text files with a newline.
| - name: wget | ||
| image: busybox | ||
| command: ['wget'] | ||
| args: ['{{ include "chat-service.fullname" . }}:{{ .Values.service.port }}'] |
There was a problem hiding this comment.
The current test only checks if a TCP connection can be established to the service port. A more robust test would be to query the /health endpoint and check for a successful HTTP response. You can do this by adjusting the wget arguments.
args: ['-q', '-O', '/dev/null', 'http://{{ include "chat-service.fullname" . }}:{{ .Values.service.port }}/health']
…ables - Remove OPENAI_API_KEY from values.yaml to prevent secrets in git - Add Secret template for secure credential management - Update deployment to use secretKeyRef for API key injection - Add comprehensive README with security best practices - Maintain backward compatibility with optional secret creation - Eliminate security vulnerability of plaintext secrets in version control 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Fix inconsistent port examples (8080 -> 8000) in port-forward help - Update comment to reflect correct default port (8000:8000) - Align examples with actual chat-service port configuration 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Fix ifndef CHART_NAME check that never triggered due to default value - Use ifeq to properly detect when CHART_NAME is still set to default - Remove trailing spaces from CHART_NAME default value - Prevent accidental port-forward attempts to non-existent example-service - Ensure users get clear error message when CHART_NAME not specified 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Change all health probes to use 'port: http' instead of hardcoded 8000 - Makes chart robust to service.port configuration changes - Update install-all and upgrade-all to use 'helm upgrade --install' - Ensures batch operations are idempotent and won't fail on re-runs - Add --create-namespace to upgrade-all for consistency - Improve overall chart reliability and user experience 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
… TCP check - Replace basic TCP connectivity test with HTTP health endpoint verification - Add --spider flag to check URL without downloading content - Add 10-second timeout to prevent test hanging - Provides more robust service health validation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…mage loading - Add CLAUDE.md with development guidance and architectural overview - Enhance Makefile with multi-environment support (dev/staging/prestaging/prod) - Create automated image loading script (load-images.sh) for CRC registry - Add comprehensive chat-service Helm chart with production-grade features: * Multi-environment values files (prestaging, staging, prod) * Enterprise templates: NetworkPolicy, PodDisruptionBudget, ResourceQuota, ServiceMonitor * Enhanced deployment with LLM configuration and security contexts * Proper secret management and environment variable handling - Update LOCAL_IMAGE_REPOSITORY.md with automated workflow documentation - Configure prestaging environment for external vLLM service connectivity - Add sample images.txt for batch image loading Successfully tested prestaging deployment with chat-service connecting to external vLLM. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
Summary
Introduces Helm chart support for the
chat-servicemicroservice and a centralizedMakefilefor managing deployments across all services.Address #98
Changes Made
Created Helm chart for
chat-serviceunderhelm/chat-service/Defined Kubernetes manifests for Deployment, Service, and Helm
NOTES.txtIntroduced
values.yamlwith configurable ports, replica count, image name, and probesAdded centralized
Makefilewith:install,upgrade,uninstall,status, andlintfor individual chartsinstall-all,upgrade-all,uninstall-allfor batch operationsport-forwardtargetIncluded examples and usage notes via
make helpUpdated
README.mdto document Makefile usage and best practicesContext / Rationale
This PR establishes production-ready deployment scaffolding for OmniPDF microservices using Helm. It enforces structure, standardization, and simplicity for both individual and multi-chart operations. The Makefile bridges the gap between local development workflows and Kubernetes-friendly deployment practices. This is the foundational step toward full CI/CD integration with Helm chart linting and versioned releases.
Related Docs or References
General Checklist