feat(charts): add Helm chart for StreamNative MCP Server #67
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a comprehensive Helm chart for deploying the StreamNative MCP Server on Kubernetes in Multi-Session Pulsar mode, along with refactoring the SSE server code to support health endpoints and updating the golangci-lint configuration.
Key changes:
- Refactored SSE server to support health and readiness endpoints (
/healthzand/readyz) - Unified HTTP server lifecycle management for both single and multi-session modes
- Added complete Helm chart with configurable deployment, service, ingress, and security settings
- Updated golangci-lint to v2.7.2 with new configuration format
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/schema/common.go | Updated comment translation from Chinese to English |
| pkg/cmd/mcp/sse.go | Major refactoring: added health endpoints, unified HTTP server management, simplified authentication middleware |
| charts/snmcp/Chart.yaml | Helm chart metadata for StreamNative MCP Server v0.1.0 |
| charts/snmcp/values.yaml | Comprehensive values configuration with Pulsar, TLS, session, and Kubernetes resource settings |
| charts/snmcp/templates/deployment.yaml | Kubernetes deployment with multi-session Pulsar mode, health probes, and security contexts |
| charts/snmcp/templates/configmap.yaml | ConfigMap for environment variable configuration |
| charts/snmcp/templates/service.yaml | Kubernetes service definition |
| charts/snmcp/templates/ingress.yaml | Optional ingress configuration |
| charts/snmcp/templates/serviceaccount.yaml | Service account template |
| charts/snmcp/templates/_helpers.tpl | Helm template helper functions |
| charts/snmcp/templates/NOTES.txt | Post-installation instructions |
| charts/snmcp/README.md | Comprehensive chart documentation with examples |
| CLAUDE.md | Updated architecture documentation with builder registry pattern and feature flags |
| AGENTS.md | Added reference to CLAUDE.md |
| .golangci.yml | Migrated to golangci-lint v2 configuration format |
| .github/workflows/lint.yaml | Updated golangci-lint version to 2.7.2 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - gosec | ||
|
|
||
| formatters: | ||
| enable: |
There was a problem hiding this comment.
The golangci-lint configuration uses a deprecated formatters section. In golangci-lint v2, formatters are not a separate section but should be managed through the output section. The gofmt and goimports should be in the linters section if they need to be explicitly enabled, though they are typically run separately as formatting tools rather than linters.
| formatters: | |
| enable: |
| labels: | ||
| {{- include "snmcp.labels" . | nindent 4 }} | ||
| data: | ||
| SNMCP_PULSAR_WEB_SERVICE_URL: {{ .Values.pulsar.webServiceURL | quote }} |
There was a problem hiding this comment.
The Helm chart does not validate that the required pulsar.webServiceURL parameter is provided. If this value is empty or not set, the deployment will fail at runtime. Consider adding a validation check in the templates or _helpers.tpl to fail at install/upgrade time with a clear error message, such as: {{- required "pulsar.webServiceURL is required" .Values.pulsar.webServiceURL }}
| SNMCP_PULSAR_WEB_SERVICE_URL: {{ .Values.pulsar.webServiceURL | quote }} | |
| SNMCP_PULSAR_WEB_SERVICE_URL: {{ required "pulsar.webServiceURL is required" .Values.pulsar.webServiceURL | quote }} |
| if r.Method != http.MethodGet { | ||
| w.WriteHeader(http.StatusMethodNotAllowed) | ||
| return | ||
| } | ||
| w.Header().Set("Content-Type", "text/plain; charset=utf-8") | ||
| w.WriteHeader(http.StatusOK) | ||
| _, _ = w.Write([]byte(status)) |
There was a problem hiding this comment.
The health handler should include a HEAD method check in addition to GET, as Kubernetes probes often use HEAD requests for efficiency. Consider changing the method check to allow both GET and HEAD: if r.Method != http.MethodGet && r.Method != http.MethodHead
| if r.Method != http.MethodGet { | |
| w.WriteHeader(http.StatusMethodNotAllowed) | |
| return | |
| } | |
| w.Header().Set("Content-Type", "text/plain; charset=utf-8") | |
| w.WriteHeader(http.StatusOK) | |
| _, _ = w.Write([]byte(status)) | |
| if r.Method != http.MethodGet && r.Method != http.MethodHead { | |
| w.WriteHeader(http.StatusMethodNotAllowed) | |
| return | |
| } | |
| w.Header().Set("Content-Type", "text/plain; charset=utf-8") | |
| w.WriteHeader(http.StatusOK) | |
| if r.Method == http.MethodGet { | |
| _, _ = w.Write([]byte(status)) | |
| } |
| ports: | ||
| - name: http | ||
| containerPort: 9090 |
There was a problem hiding this comment.
The containerPort is hardcoded to 9090, but the server.httpAddr can be configured to use a different port. This mismatch could cause the service to not route traffic correctly. The containerPort should be dynamically extracted from server.httpAddr or server.httpAddr should be validated to ensure it uses port 9090. Consider parsing the port from httpAddr or documenting that the port in httpAddr must match 9090.
| ports: | |
| - name: http | |
| containerPort: 9090 | |
| {{- $httpPort := 9090 }} | |
| {{- with .Values.server.httpAddr }} | |
| {{- $match := regexFind "[0-9]+$" . }} | |
| {{- if $match }} | |
| {{- $httpPort = int $match }} | |
| {{- end }} | |
| {{- end }} | |
| ports: | |
| - name: http | |
| containerPort: {{ $httpPort }} |
No description provided.