[deploy] add Helm chart #182
Conversation
WalkthroughAdds a new Helm chart for sms-gateway (Chart, templates, values, README, NOTES, tests), an end-to-end Minikube test script, and a GitHub Actions workflow to package and publish the chart to an S3-backed Helm repository. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub Actions
participant Runner as Runner (Helm/CLI)
participant S3 as AWS S3 (helm-s3)
Dev->>GH: Push to master under deployments/helm-chart/** or publish release
GH->>Runner: Checkout repo, configure permissions & AWS OIDC/creds
Runner->>Runner: Setup Helm, install helm-s3 plugin
Runner->>Runner: Determine chart version (release tag or commit/dev) and helm package
Runner->>S3: helm s3 init/add repo if missing
Runner->>S3: helm s3 push <chart.tgz> s3://bucket/repo
Runner-->>GH: Remove artifacts, finish workflow
sequenceDiagram
autonumber
actor Operator as Cluster Operator
participant Helm as Helm CLI
participant K8s as Kubernetes API
participant App as sms-gateway
participant DB as MariaDB
Operator->>Helm: helm install/upgrade sms-gateway -f values.yaml
Helm->>K8s: Apply manifests (ServiceAccount, Secret, Service, Deployment, ...)
alt database.deployInternal = true
K8s->>K8s: Create StatefulSet, Service, PVC, Secret for MariaDB
App->>DB: Connect via internal DB service
else
App->>DB: Connect to external DB host/port from values/secret
end
K8s->>App: Pods become Ready
Operator->>App: Access via Ingress/NodePort/LoadBalancer/port-forward (per NOTES)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
deployments/helm-chart/README.md (1)
54-57: Align docs with actual values keyThe chart templates expect
gateway.fcmCredentials, but the table documentsgateway.fcmKey. Please update the docs (or the values) so the key matches reality; otherwise users will set the wrong field.deployments/helm-chart/values.yaml (1)
73-73: Database host should reference the internal service name.When
database.deployInternalis true, the database host should match the generated service name pattern (i.e.,<fullname>-db) rather than a staticdbvalue. The deployment template correctly handles this at lines 44-50 of deployment.yaml, but the default value here is misleading.Consider updating the default or adding a comment:
database: - host: db + host: "" # Auto-configured when deployInternal is true, otherwise set your external DB host port: 3306deployments/helm-chart/templates/database.yaml (2)
46-57: Consider adding initial delay to probes.The readiness and liveness probes start immediately, which might cause unnecessary failures during MariaDB initialization, especially for the first startup.
Add initial delays:
readinessProbe: + initialDelaySeconds: 30 + periodSeconds: 10 exec: command: - healthcheck.sh - --connect - --innodb_initialized livenessProbe: + initialDelaySeconds: 60 + periodSeconds: 30 exec: command: - healthcheck.sh - --connect - --innodb_initialized
102-106: Consider making storage class configurable.The PersistentVolumeClaim doesn't specify a
storageClassName, which means it will use the cluster's default storage class. This may not be suitable for all environments.Add a configurable storage class in values.yaml and reference it here:
In values.yaml:
database: mariadb: persistence: enabled: true size: 8Gi storageClass: "" # Use default if emptyIn database.yaml:
resources: requests: storage: {{ .Values.database.mariadb.persistence.size }} + {{- if .Values.database.mariadb.persistence.storageClass }} + storageClassName: {{ .Values.database.mariadb.persistence.storageClass }} + {{- end }} {{- end }}deployments/helm-chart/templates/deployment.yaml (2)
83-90: Add initial delays and timeouts to health probes.The health probes lack initial delay and timeout configurations, which could cause premature failures during application startup or when the application is under load.
Add probe configuration:
livenessProbe: + initialDelaySeconds: 30 + periodSeconds: 10 + timeoutSeconds: 5 + failureThreshold: 3 httpGet: path: /health port: http readinessProbe: + initialDelaySeconds: 10 + periodSeconds: 5 + timeoutSeconds: 3 + failureThreshold: 3 httpGet: path: /health port: http
60-76: Verify mutual exclusivity of gateway modes.The template assumes that either
privateTokenorfcmCredentialsis set, but doesn't handle the case where both are set or neither is set. This could lead to an incomplete deployment configuration.Consider adding validation or documentation:
- Add a comment explaining the modes:
{{- if .Values.gateway.privateToken }} # Private gateway mode - uses private token for authentication - name: GATEWAY__MODE
- Consider adding a validation check that fails deployment if the configuration is invalid. Would you like me to help generate a validation helper that checks these conditions?
scripts/test-helm-minikube.sh (2)
66-69: Consider less aggressive cleanup option.Deleting the entire Minikube cluster (
minikube delete) is aggressive and will destroy all resources, not just the test namespace. This could be problematic if the user has other workloads in the cluster.Consider only deleting the namespace:
# Cleanup echo "Cleaning up..." kill $PORT_FORWARD_PID -minikube delete +kubectl delete namespace $NAMESPACE +echo "Note: Minikube cluster is still running. Run 'minikube delete' to remove it."Or make it optional:
# Cleanup echo "Cleaning up..." kill $PORT_FORWARD_PID + +# Delete namespace +kubectl delete namespace $NAMESPACE + +# Optionally delete Minikube cluster +read -p "Delete Minikube cluster? (y/N): " -n 1 -r +echo +if [[ $REPLY =~ ^[Yy]$ ]]; then + minikube delete +fi
50-56: Improve reliability of port-forward setup.The script uses a fixed 5-second sleep to wait for port-forward, which may be insufficient on slower systems or excessive on faster ones.
Consider actively waiting for the port:
# Port forward to access the service echo "Port forwarding to service (http://localhost:8080)..." kubectl port-forward --namespace $NAMESPACE service/sms-gateway-test 8080:3000 & PORT_FORWARD_PID=$! -# Give it a moment to establish the connection -sleep 5 +# Wait for port-forward to be ready +echo "Waiting for port-forward to be ready..." +for i in {1..30}; do + if curl -s http://localhost:8080/health > /dev/null 2>&1; then + echo "Port-forward is ready!" + break + fi + if [ $i -eq 30 ]; then + echo "Error: Port-forward failed to establish" + kill $PORT_FORWARD_PID + exit 1 + fi + sleep 1 +done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/workflows/publish-helm-chart.yml(1 hunks)deployments/helm-chart/Chart.yaml(1 hunks)deployments/helm-chart/README.md(1 hunks)deployments/helm-chart/templates/NOTES.txt(1 hunks)deployments/helm-chart/templates/_helpers.tpl(1 hunks)deployments/helm-chart/templates/database.yaml(1 hunks)deployments/helm-chart/templates/deployment.yaml(1 hunks)deployments/helm-chart/templates/hpa.yaml(1 hunks)deployments/helm-chart/templates/ingress.yaml(1 hunks)deployments/helm-chart/templates/secrets.yaml(1 hunks)deployments/helm-chart/templates/service.yaml(1 hunks)deployments/helm-chart/templates/serviceaccount.yaml(1 hunks)deployments/helm-chart/templates/tests/test-connection.yaml(1 hunks)deployments/helm-chart/values.yaml(1 hunks)scripts/test-helm-minikube.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-05T08:37:28.077Z
Learnt from: eznix86
PR: android-sms-gateway/server#172
File: deployments/kubernetes/sms-gateway-server/Chart.yaml:9-10
Timestamp: 2025-09-05T08:37:28.077Z
Learning: In the android-sms-gateway/server project, the Helm chart's appVersion in Chart.yaml is set to "latest" as a placeholder because CI automatically updates it with the actual version from git tags during the release process via the bump-chart-app-version.yaml workflow.
Applied to files:
deployments/helm-chart/README.mddeployments/helm-chart/Chart.yaml
🪛 Shellcheck (0.11.0)
scripts/test-helm-minikube.sh
[warning] 27-27: Quote this to prevent word splitting.
(SC2046)
🪛 YAMLlint (1.37.1)
deployments/helm-chart/templates/service.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/hpa.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/secrets.yaml
[error] 4-4: syntax error: expected , but found ''
(syntax)
deployments/helm-chart/templates/ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/database.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/serviceaccount.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/tests/test-connection.yaml
[error] 4-4: syntax error: expected , but found ''
(syntax)
deployments/helm-chart/templates/deployment.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
deployments/helm-chart/values.yaml (1)
1-99: LGTM overall structure!The values.yaml structure is well-organized with comprehensive configuration options for deployment, service, ingress, resources, autoscaling, database, and gateway settings. The defaults are reasonable for development/testing scenarios.
deployments/helm-chart/templates/hpa.yaml (1)
1-28: LGTM!The HorizontalPodAutoscaler template is correctly configured with:
- Proper conditional rendering based on
.Values.autoscaling.enabled- autoscaling/v2 API version
- Both CPU and memory utilization metrics
- Correct reference to the target deployment
Note: The YAMLlint syntax error is a false positive caused by Helm template syntax (the
{{-delimiter).deployments/helm-chart/templates/NOTES.txt (1)
1-33: Well-structured post-deployment guidance.The NOTES.txt provides comprehensive instructions for accessing the application across different service types and includes helpful health check and database information.
deployments/helm-chart/templates/database.yaml (1)
1-107: Well-structured database deployment template.The database.yaml correctly configures a StatefulSet with:
- Proper secret references for passwords
- Health checks using MariaDB's built-in healthcheck script
- Persistent volume for data
- Appropriate labels and selectors
deployments/helm-chart/templates/deployment.yaml (1)
1-102: Excellent deployment template structure!The deployment.yaml is well-configured with:
- Proper environment variable handling for both internal and external database modes
- Conditional gateway mode configuration
- Correct secret references
- Resource limits and security contexts
- Comprehensive pod scheduling options (nodeSelector, affinity, tolerations)
scripts/test-helm-minikube.sh (1)
1-72: Well-structured test automation script!The script provides comprehensive end-to-end testing with proper error handling, prerequisite checks, and clear progress output. The overall flow is logical and helpful for testing the Helm chart.
deployments/helm-chart/templates/_helpers.tpl (1)
1-42: LGTM! Standard Helm helpers implementation.The helper templates follow Helm best practices with:
- Proper fullname generation with override support and truncation
- Standard label templates for consistent resource labeling
- Service account name resolution logic
- Correct handling of chart versioning in labels
These helpers provide the foundation for consistent naming and labeling across all chart resources.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/publish-helm-chart.yml(1 hunks)deployments/helm-chart/Chart.yaml(1 hunks)deployments/helm-chart/README.md(1 hunks)deployments/helm-chart/templates/NOTES.txt(1 hunks)deployments/helm-chart/templates/database.yaml(1 hunks)deployments/helm-chart/templates/deployment.yaml(1 hunks)deployments/helm-chart/values.yaml(1 hunks)scripts/test-helm-minikube.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- deployments/helm-chart/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- deployments/helm-chart/values.yaml
- deployments/helm-chart/Chart.yaml
- scripts/test-helm-minikube.sh
🧰 Additional context used
🪛 YAMLlint (1.37.1)
deployments/helm-chart/templates/database.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/deployment.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E
- GitHub Check: Analyze (go)
capcom6
force-pushed
the
deploy/helm-chart
branch
from
b87ed07 to
6123028
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
deployments/helm-chart/templates/tests/test-connection.yaml (1)
7-14: Add a hook-delete policy for Helm tests.Without a
helm.sh/hook-delete-policyannotation, successful test Pods linger unless the operator passeshelm test --cleanup. Addbefore-hook-creation,hook-succeededso repeated test runs stay tidy.annotations: "helm.sh/hook": test + "helm.sh/hook-delete-policy": before-hook-creation,hook-succeededdeployments/helm-chart/templates/database.yaml (1)
62-113: Prefer StatefulSet volumeClaimTemplates over detached PVCs.Hard-wiring a single PVC with
claimNamecouples the StatefulSet to a pre-created volume and blocks any scale-out or re-creation without manual cleanup. Define the storage involumeClaimTemplatesso Kubernetes provisions per-pod PVCs automatically.- volumes: - - name: mariadb-data - persistentVolumeClaim: - claimName: {{ include "sms-gateway.fullname" . }}-db-pvc + volumeClaimTemplates: + - metadata: + name: mariadb-data + spec: + accessModes: + - ReadWriteOnce + resources: + requests: + storage: {{ .Values.database.mariadb.persistence.size }} + {{- if .Values.database.mariadb.persistence.storageClass }} + storageClassName: {{ .Values.database.mariadb.persistence.storageClass }} + {{- end }}And drop the standalone PVC manifest below.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/workflows/publish-helm-chart.yml(1 hunks)deployments/helm-chart/Chart.yaml(1 hunks)deployments/helm-chart/README.md(1 hunks)deployments/helm-chart/templates/NOTES.txt(1 hunks)deployments/helm-chart/templates/_helpers.tpl(1 hunks)deployments/helm-chart/templates/database.yaml(1 hunks)deployments/helm-chart/templates/deployment.yaml(1 hunks)deployments/helm-chart/templates/hpa.yaml(1 hunks)deployments/helm-chart/templates/ingress.yaml(1 hunks)deployments/helm-chart/templates/secrets.yaml(1 hunks)deployments/helm-chart/templates/service.yaml(1 hunks)deployments/helm-chart/templates/serviceaccount.yaml(1 hunks)deployments/helm-chart/templates/tests/test-connection.yaml(1 hunks)deployments/helm-chart/values.yaml(1 hunks)scripts/test-helm-minikube.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- deployments/helm-chart/values.yaml
- deployments/helm-chart/README.md
- scripts/test-helm-minikube.sh
- deployments/helm-chart/templates/_helpers.tpl
- .github/workflows/publish-helm-chart.yml
- deployments/helm-chart/Chart.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
deployments/helm-chart/templates/secrets.yaml
[error] 4-4: syntax error: expected , but found ''
(syntax)
deployments/helm-chart/templates/tests/test-connection.yaml
[error] 4-4: syntax error: expected , but found ''
(syntax)
deployments/helm-chart/templates/hpa.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/database.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/deployment.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/service.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/serviceaccount.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
deployments/helm-chart/templates/NOTES.txt(1 hunks)deployments/helm-chart/templates/ingress.yaml(1 hunks)deployments/helm-chart/templates/tests/test-connection.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
deployments/helm-chart/templates/ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/tests/test-connection.yaml
[error] 4-4: syntax error: expected , but found ''
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E
- GitHub Check: Analyze (go)
capcom6
force-pushed
the
deploy/helm-chart
branch
from
9f650d0 to
5900d8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
deployments/helm-chart/templates/tests/test-connection.yaml (1)
13-13: Pin the curl test image.Please replace
curlimages/curl:latestwith a pinned version (ideally with digest) so chart tests remain reproducible and avoid supply-chain drift.
🧹 Nitpick comments (2)
deployments/helm-chart/templates/database.yaml (1)
31-35: Quote MariaDB env values to keep them strings.
MARIADB_DATABASEandMARIADB_USERare emitted as plain scalars. If someone sets these to values that parse as numbers or booleans (0,true, etc.), Helm will hand Kubernetes a non-string and the pod spec will be rejected becauseEnvVar.valuemust be a string. Please add| quoteto these assignments so they remain valid for any user-provided input.Apply:
- - name: MARIADB_DATABASE - value: {{ .Values.database.name }} - - name: MARIADB_USER - value: {{ .Values.database.user }} + - name: MARIADB_DATABASE + value: {{ .Values.database.name | quote }} + - name: MARIADB_USER + value: {{ .Values.database.user | quote }}deployments/helm-chart/templates/deployment.yaml (1)
51-55: Keep database env vars quoted here too.Same concern as in the StatefulSet: leaving
DATABASE__NAMEandDATABASE__USERunquoted lets YAML coerce user-supplied values into numbers/booleans, which violates the Kubernetes schema forenv.value. Please add| quoteso any input stays a string.Apply:
- - name: DATABASE__NAME - value: {{ .Values.database.name }} - - name: DATABASE__USER - value: {{ .Values.database.user }} + - name: DATABASE__NAME + value: {{ .Values.database.name | quote }} + - name: DATABASE__USER + value: {{ .Values.database.user | quote }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/workflows/publish-helm-chart.yml(1 hunks)deployments/helm-chart/Chart.yaml(1 hunks)deployments/helm-chart/README.md(1 hunks)deployments/helm-chart/templates/NOTES.txt(1 hunks)deployments/helm-chart/templates/_helpers.tpl(1 hunks)deployments/helm-chart/templates/database.yaml(1 hunks)deployments/helm-chart/templates/deployment.yaml(1 hunks)deployments/helm-chart/templates/hpa.yaml(1 hunks)deployments/helm-chart/templates/ingress.yaml(1 hunks)deployments/helm-chart/templates/secrets.yaml(1 hunks)deployments/helm-chart/templates/service.yaml(1 hunks)deployments/helm-chart/templates/serviceaccount.yaml(1 hunks)deployments/helm-chart/templates/tests/test-connection.yaml(1 hunks)deployments/helm-chart/values.yaml(1 hunks)scripts/test-helm-minikube.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- deployments/helm-chart/Chart.yaml
- deployments/helm-chart/README.md
- .github/workflows/publish-helm-chart.yml
- deployments/helm-chart/templates/_helpers.tpl
- scripts/test-helm-minikube.sh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-05T08:37:28.077Z
Learnt from: eznix86
PR: android-sms-gateway/server#172
File: deployments/kubernetes/sms-gateway-server/Chart.yaml:9-10
Timestamp: 2025-09-05T08:37:28.077Z
Learning: In the android-sms-gateway/server project, the Helm chart's appVersion in Chart.yaml is set to "latest" as a placeholder because CI automatically updates it with the actual version from git tags during the release process via the bump-chart-app-version.yaml workflow.
Applied to files:
deployments/helm-chart/values.yaml
🪛 YAMLlint (1.37.1)
deployments/helm-chart/templates/database.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/deployment.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/hpa.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/secrets.yaml
[error] 4-4: syntax error: expected , but found ''
(syntax)
deployments/helm-chart/templates/service.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/serviceaccount.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/tests/test-connection.yaml
[error] 4-4: syntax error: expected , but found ''
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
|
Published successfully... |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
deployments/helm-chart/Chart.yaml (1)
6-6: Restore the placeholderappVersion.CI bumps
appVersionfrom git tags during release; committing"1.29.3"breaks that automation. Please revert this field to"latest"so the workflow can keep managing it.
Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/publish-helm-chart.yml(1 hunks)deployments/helm-chart/Chart.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-05T08:37:28.077Z
Learnt from: eznix86
PR: android-sms-gateway/server#172
File: deployments/kubernetes/sms-gateway-server/Chart.yaml:9-10
Timestamp: 2025-09-05T08:37:28.077Z
Learning: In the android-sms-gateway/server project, the Helm chart's appVersion in Chart.yaml is set to "latest" as a placeholder because CI automatically updates it with the actual version from git tags during the release process via the bump-chart-app-version.yaml workflow.
Applied to files:
deployments/helm-chart/Chart.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/publish-helm-chart.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E
- GitHub Check: Analyze (go)
capcom6
force-pushed
the
deploy/helm-chart
branch
from
442f69f to
8223c20
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
deployments/helm-chart/templates/tests/test-connection.yaml (1)
11-15: Pin test image and add retry for startup raceUse a fixed curl image tag and retry until service is up to avoid flaky tests.
containers: - name: curl - image: curlimages/curl:latest - command: ['sh', '-c', 'curl --fail http://{{ include "sms-gateway.fullname" . }}:{{ .Values.service.port }}/health'] + image: curlimages/curl:8.11.0 + command: + - sh + - -c + - >- + for i in $(seq 1 30); do + curl --fail --silent --show-error + http://{{ include "sms-gateway.fullname" . }}:{{ .Values.service.port }}/health && exit 0; + sleep 2; + done; + echo "Service not healthy in time"; exit 1deployments/helm-chart/templates/NOTES.txt (3)
6-7: (ack) Guard against empty ingress paths handled.You added a safe default for missing
.paths. Looks good.
29-31: (ack) Avoid printing root password in NOTES.Password is no longer echoed; references Secret instead. Good.
15-16: (ack) Handle LoadBalancer hostname/IP.Concatenation approach covers providers exposing either field.
🧹 Nitpick comments (9)
.github/workflows/publish-helm-chart.yml (2)
51-56: Initialize the S3 Helm repo if missing (idempotent)If the bucket hasn’t been initialized, helm s3 push will fail. Add an idempotent init.
- name: Initialize S3 repository run: | - if ! helm repo list | grep s3-repo; then - helm repo add s3-repo s3://${{ secrets.AWS_BUCKET }}/charts - fi + # Initialize remote repo (no-op if already initialized) + helm s3 init s3://${{ secrets.AWS_BUCKET }}/charts || true + # Add repo locally if not present + if ! helm repo list | grep -q '^s3-repo'; then + helm repo add s3-repo s3://${{ secrets.AWS_BUCKET }}/charts + fi
32-50: Lint the chart before packaging to catch template errors earlyAdd helm lint to fail fast on template issues.
- name: Set up Helm uses: azure/setup-helm@v4 + + - name: Lint Helm chart + run: helm lint deployments/helm-chart @@ fi helm package deployments/helm-chart --version "${CHART_VERSION}" --app-version "${APP_VERSION}"deployments/helm-chart/templates/secrets.yaml (1)
8-15: Prefer stringData and enforce required values (simpler, avoids base64 footguns).Using
data+b64encis error‑prone for operators. UsestringDatawith plaintext values and enforce presence of required fields (e.g., DB password). Kubernetes will base64‑encode for you.Apply:
type: Opaque -data: - database-password: {{ .Values.database.password | b64enc | quote }} - {{- if .Values.gateway.privateToken }} - private-token: {{ .Values.gateway.privateToken | b64enc | quote }} - {{- end }} - {{- if .Values.gateway.fcmCredentials }} - fcm-credentials: {{ .Values.gateway.fcmCredentials | b64enc | quote }} - {{- end }} +stringData: + database-password: {{ required "values.database.password is required" .Values.database.password | quote }} + {{- if .Values.gateway.privateToken }} + private-token: {{ .Values.gateway.privateToken | quote }} + {{- end }} + {{- if .Values.gateway.fcmCredentials }} + fcm-credentials: {{ .Values.gateway.fcmCredentials | quote }} + {{- end }}deployments/helm-chart/templates/database.yaml (3)
62-66: Use StatefulSet volumeClaimTemplates instead of standalone PVC.A StatefulSet should own per‑pod storage via
volumeClaimTemplates; a separate PVC couples a single claim to all replicas and complicates scaling.Apply:
@@ - volumes: - - name: mariadb-data - persistentVolumeClaim: - claimName: {{ include "sms-gateway.fullname" . }}-db-pvc + volumeClaimTemplates: + - metadata: + name: mariadb-data + labels: + {{- include "sms-gateway.labels" . | nindent 12 }} + spec: + accessModes: ["ReadWriteOnce"] + resources: + requests: + storage: {{ .Values.database.mariadb.persistence.size }} + {{- if .Values.database.mariadb.persistence.storageClass }} + storageClassName: {{ .Values.database.mariadb.persistence.storageClass }} + {{- end }} @@ ---- -# Database Persistent Volume Claim -apiVersion: v1 -kind: PersistentVolumeClaim -metadata: - name: {{ include "sms-gateway.fullname" . }}-db-pvc - labels: - {{- include "sms-gateway.labels" . | nindent 4 }} -spec: - accessModes: - - ReadWriteOnce - resources: - requests: - storage: {{ .Values.database.mariadb.persistence.size }} - {{- if .Values.database.mariadb.persistence.storageClass }} - storageClassName: {{ .Values.database.mariadb.persistence.storageClass }} - {{- end }}Also applies to: 97-113
84-96: Avoid duplicating DB password across multiple Secrets.The DB password is stored in both
...-db-secretsand...-secrets, increasing exposure surface. Consider using a single Secret fordatabase-passwordand reference it from both Deployment and StatefulSet (keeproot-passwordonly in the DB Secret).
93-95: Secret material in values: recommend externalization.Storing
rootPasswordand DB password invalues.yamlis common but risky. Prefer:
- Helm secrets/SealedSecrets, or
- External Secrets Operator, or
--set-fileto load from CI secrets at install time.deployments/helm-chart/templates/NOTES.txt (1)
14-16: Tiny wording tweak for clarity.“status of by running” → “status by running”.
Apply:
- You can watch the status of by running 'kubectl get --namespace {{ .Release.Namespace }} svc -w {{ include "sms-gateway.fullname" . }}' + You can watch the status by running 'kubectl get --namespace {{ .Release.Namespace }} svc -w {{ include "sms-gateway.fullname" . }}'deployments/helm-chart/templates/deployment.yaml (2)
51-55: Gate CONFIG_PATH env with config.enabled to avoid dangling path.If the config volume is disabled, pointing to
/app/config.ymlmay confuse the app. Only setCONFIG_PATHwhen the volume is mounted.Apply:
- - name: CONFIG_PATH - value: "/app/config.yml" + {{- if .Values.gateway.config.enabled }} + - name: CONFIG_PATH + value: "/app/config.yml" + {{- end }}Also applies to: 110-115, 30-39
71-87: Validate gateway mode inputs (exactly one of privateToken or fcmCredentials).To prevent misconfig, add a template guard to fail when both or neither are set.
Add near the top of the template (or a separate _validate.tpl):
{{- if and .Values.gateway.privateToken .Values.gateway.fcmCredentials -}} {{- fail "Provide only one of gateway.privateToken or gateway.fcmCredentials, not both." -}} {{- end -}} {{- if not (or .Values.gateway.privateToken .Values.gateway.fcmCredentials) -}} {{- fail "Provide one of gateway.privateToken or gateway.fcmCredentials." -}} {{- end -}}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.github/workflows/publish-helm-chart.yml(1 hunks)deployments/helm-chart/Chart.yaml(1 hunks)deployments/helm-chart/README.md(1 hunks)deployments/helm-chart/templates/NOTES.txt(1 hunks)deployments/helm-chart/templates/_helpers.tpl(1 hunks)deployments/helm-chart/templates/configmap.yaml(1 hunks)deployments/helm-chart/templates/database.yaml(1 hunks)deployments/helm-chart/templates/deployment.yaml(1 hunks)deployments/helm-chart/templates/hpa.yaml(1 hunks)deployments/helm-chart/templates/ingress.yaml(1 hunks)deployments/helm-chart/templates/secrets.yaml(1 hunks)deployments/helm-chart/templates/service.yaml(1 hunks)deployments/helm-chart/templates/serviceaccount.yaml(1 hunks)deployments/helm-chart/templates/tests/test-connection.yaml(1 hunks)deployments/helm-chart/values.yaml(1 hunks)scripts/test-helm-minikube.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- deployments/helm-chart/Chart.yaml
- deployments/helm-chart/templates/_helpers.tpl
- scripts/test-helm-minikube.sh
- deployments/helm-chart/values.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-05T08:37:28.077Z
Learnt from: eznix86
PR: android-sms-gateway/server#172
File: deployments/kubernetes/sms-gateway-server/Chart.yaml:9-10
Timestamp: 2025-09-05T08:37:28.077Z
Learning: In the android-sms-gateway/server project, the Helm chart's appVersion in Chart.yaml is set to "latest" as a placeholder because CI automatically updates it with the actual version from git tags during the release process via the bump-chart-app-version.yaml workflow.
Applied to files:
deployments/helm-chart/README.md
🧬 Code graph analysis (1)
deployments/helm-chart/templates/configmap.yaml (1)
internal/config/config.go (1)
Gateway(10-18)
🪛 LanguageTool
deployments/helm-chart/templates/NOTES.txt
[grammar] ~19-~19: There might be a mistake here.
Context: ....containers[0].ports[0].containerPort}") echo "Visit http://127.0.0.1:8080 to use...
(QB_NEW_EN)
[grammar] ~21-~21: There might be a mistake here.
Context: ...espace }} $POD_NAME 8080:$CONTAINER_PORT {{- end }} 2. Check the application hea...
(QB_NEW_EN)
[grammar] ~24-~24: There might be a mistake here.
Context: ...end }} 2. Check the application health: curl http://
(QB_NEW_EN)
deployments/helm-chart/README.md
[grammar] ~7-~7: There might be a mistake here.
Context: ...s. ## Prerequisites - Kubernetes 1.19+ - Helm 3.2.0+ - PV provisioner support in ...
(QB_NEW_EN)
[grammar] ~8-~8: There might be a mistake here.
Context: ...isites - Kubernetes 1.19+ - Helm 3.2.0+ - PV provisioner support in the underlying...
(QB_NEW_EN)
🪛 YAMLlint (1.37.1)
deployments/helm-chart/templates/database.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/deployment.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/secrets.yaml
[error] 4-4: syntax error: expected , but found ''
(syntax)
deployments/helm-chart/templates/configmap.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/hpa.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/service.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/serviceaccount.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/tests/test-connection.yaml
[error] 4-4: syntax error: expected , but found ''
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
deployments/helm-chart/templates/ingress.yaml (1)
8-11: Good fix: annotations are now properly guardedWrapping annotations with with prevents emitting null maps. LGTM.
.github/workflows/publish-helm-chart.yml (1)
57-63: No changes required for Helm chart artifact glob
Chart.yaml’s name: server means server-.tgz already matches server-.tgz; the suggested ./.tgz glob is unnecessary.Likely an incorrect or invalid review comment.
deployments/helm-chart/templates/secrets.yaml (1)
5-6: YAMLlint error likely false positive; please helm-lint to confirm.The linter error at metadata/labels is commonly due to Go templates. Rendering looks fine (
nindent 4underlabels:). Please runhelm lintandhelm templateto verify.deployments/helm-chart/templates/database.yaml (1)
1-1: YAMLlint error is due to templating; please verify with Helm.The “found '-'” error occurs with leading template trimming. Validate with
helm lintandhelm template.deployments/helm-chart/templates/deployment.yaml (1)
6-6: YAMLlint error likely due to templating; please helm-lint.The reported syntax error is common with Go templates. Confirm with
helm lint/helm template.
capcom6
force-pushed
the
deploy/helm-chart
branch
from
4d458c9 to
837effa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
deployments/helm-chart/templates/tests/test-connection.yaml (1)
13-13: Pin the curl test image for reproducibility and supply‑chain safety.Floating tags drift and can break tests or introduce CVEs. Pin to a specific version (or digest).
- image: curlimages/curl:latest + image: curlimages/curl:8.11.0.github/workflows/publish-helm-chart.yml (1)
60-66: Fix the chart artifact glob before pushing and cleaning up.The packaged filename may not match
server-*.tgz. Use a broad glob to avoid “file not found” and failed pushes.- helm s3 push server-*.tgz s3-repo + helm s3 push *.tgz s3-repo @@ - rm -f server-*.tgz index.yaml + rm -f *.tgzdeployments/helm-chart/templates/hpa.yaml (1)
1-28: HPA apiVersion incompatible with Kubernetes 1.19-1.22 clusters.This issue was previously identified: the template uses
autoscaling/v2, which became stable only in Kubernetes 1.23. Clusters running 1.19-1.22 requireautoscaling/v2beta2. The chart will fail to install on older supported clusters without conditional apiVersion logic.deployments/helm-chart/templates/database.yaml (1)
46-61: MariaDB probes reference non-existent healthcheck.sh script.This issue was previously identified: standard MariaDB images don't ship
healthcheck.sh. The readiness and liveness probes will fail, preventing pods from becoming ready. UsetcpSocketon port 3306 ormysqladmin pingvia exec instead.
🧹 Nitpick comments (4)
deployments/helm-chart/templates/tests/test-connection.yaml (1)
14-14: Harden the curl test for flaky clusters.Add timeout/retry flags to reduce false negatives during transient DNS/startup delays.
- command: ['sh', '-c', 'curl --fail http://{{ include "sms-gateway.fullname" . }}:{{ .Values.service.port }}/health'] + command: ['sh', '-c', 'curl --fail --show-error --silent --connect-timeout 5 --max-time 10 --retry 5 --retry-all-errors http://{{ include "sms-gateway.fullname" . }}:{{ .Values.service.port }}/health'].github/workflows/publish-helm-chart.yml (1)
18-21: Prevent concurrent publishes from racing.Add a concurrency group so only one publish runs at a time, avoiding index update races.
jobs: publish: runs-on: ubuntu-latest + concurrency: + group: helm-publish + cancel-in-progress: falseAlso applies to: 28-31
deployments/helm-chart/README.md (1)
31-62: Document additional values exposed by the chart.Add serviceAccount.* and ingress.annotations to the table for discoverability.
- serviceAccount.create (bool)
- serviceAccount.name (string)
- serviceAccount.annotations (map)
- ingress.annotations (map)
deployments/helm-chart/templates/secrets.yaml (1)
9-15: Improve secret ergonomics and safety.
- Prefer stringData to avoid manual/base64 transforms.
- Optionally support an existingSecret to keep sensitive data out of values.
- Consider requiring a non-empty database password to avoid deploying with blank creds.
Example change (stringData + required):
-kind: Secret +kind: Secret @@ -type: Opaque -data: - database-password: {{ .Values.database.password | b64enc | quote }} +type: Opaque +stringData: + database-password: {{ required "database.password is required" .Values.database.password | quote }} {{- if .Values.gateway.privateToken }} - private-token: {{ .Values.gateway.privateToken | b64enc | quote }} + private-token: {{ .Values.gateway.privateToken | quote }} {{- end }} {{- if .Values.gateway.fcmCredentials }} - fcm-credentials: {{ .Values.gateway.fcmCredentials | b64enc | quote }} + fcm-credentials: {{ .Values.gateway.fcmCredentials | quote }} {{- end }}If you prefer an existingSecret option, I can draft that pattern across deployment mounts too.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.github/workflows/publish-helm-chart.yml(1 hunks)deployments/helm-chart/Chart.yaml(1 hunks)deployments/helm-chart/README.md(1 hunks)deployments/helm-chart/templates/NOTES.txt(1 hunks)deployments/helm-chart/templates/_helpers.tpl(1 hunks)deployments/helm-chart/templates/configmap.yaml(1 hunks)deployments/helm-chart/templates/database.yaml(1 hunks)deployments/helm-chart/templates/deployment.yaml(1 hunks)deployments/helm-chart/templates/hpa.yaml(1 hunks)deployments/helm-chart/templates/ingress.yaml(1 hunks)deployments/helm-chart/templates/secrets.yaml(1 hunks)deployments/helm-chart/templates/service.yaml(1 hunks)deployments/helm-chart/templates/serviceaccount.yaml(1 hunks)deployments/helm-chart/templates/tests/test-connection.yaml(1 hunks)deployments/helm-chart/values.yaml(1 hunks)scripts/test-helm-minikube.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- deployments/helm-chart/Chart.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-05T08:37:28.077Z
Learnt from: eznix86
PR: android-sms-gateway/server#172
File: deployments/kubernetes/sms-gateway-server/Chart.yaml:9-10
Timestamp: 2025-09-05T08:37:28.077Z
Learning: In the android-sms-gateway/server project, the Helm chart's appVersion in Chart.yaml is set to "latest" as a placeholder because CI automatically updates it with the actual version from git tags during the release process via the bump-chart-app-version.yaml workflow.
Applied to files:
deployments/helm-chart/values.yamldeployments/helm-chart/README.md
🧬 Code graph analysis (1)
deployments/helm-chart/templates/configmap.yaml (1)
internal/config/config.go (1)
Gateway(10-18)
🪛 LanguageTool
deployments/helm-chart/README.md
[grammar] ~7-~7: There might be a mistake here.
Context: ...s. ## Prerequisites - Kubernetes 1.23+ - Helm 3.2.0+ - PV provisioner support in ...
(QB_NEW_EN)
[grammar] ~8-~8: There might be a mistake here.
Context: ...isites - Kubernetes 1.23+ - Helm 3.2.0+ - PV provisioner support in the underlying...
(QB_NEW_EN)
deployments/helm-chart/templates/NOTES.txt
[grammar] ~19-~19: There might be a mistake here.
Context: ....containers[0].ports[0].containerPort}") echo "Visit http://127.0.0.1:8080 to use...
(QB_NEW_EN)
[grammar] ~21-~21: There might be a mistake here.
Context: ...espace }} $POD_NAME 8080:$CONTAINER_PORT {{- end }} 2. Check the application hea...
(QB_NEW_EN)
[grammar] ~24-~24: There might be a mistake here.
Context: ...end }} 2. Check the application health: curl http://
(QB_NEW_EN)
🪛 YAMLlint (1.37.1)
deployments/helm-chart/templates/serviceaccount.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/configmap.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/secrets.yaml
[error] 4-4: syntax error: expected , but found ''
(syntax)
deployments/helm-chart/templates/ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/hpa.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/database.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/deployment.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/service.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/tests/test-connection.yaml
[error] 4-4: syntax error: expected , but found ''
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (15)
deployments/helm-chart/templates/ingress.yaml (1)
8-11: LGTM on annotations guard and general wiring.Annotations are properly wrapped; rules/TLS blocks look correct.
deployments/helm-chart/README.md (1)
15-19: Install command chart name is correct:sms-gate/servermatches thename: serverin Chart.yaml.deployments/helm-chart/templates/service.yaml (1)
15-17: No action required—container port "http" is correctly named in the Deployment.Verification confirms the Deployment (deployment.yaml:47) defines a container port named "http", which correctly matches the Service's targetPort reference (service.yaml:15). Port wiring is valid and traffic will route as expected.
deployments/helm-chart/values.yaml (2)
5-8: LGTM: Specific version tag used instead of "latest".The image tag now specifies version
1.29.3instead oflatest, which is appropriate for production deployments and enables predictable rollbacks.
74-74: LGTM: Security comments added for sensitive fields.The comments clearly indicate that these password and credential fields are required and must be set to strong values, which addresses the security documentation concern.
Also applies to: 82-82, 90-91
deployments/helm-chart/templates/NOTES.txt (4)
6-6: LGTM: Empty paths array guarded.The conditional
{{ if .paths }}...{{ else }}/{{ end }}now prevents template errors when the paths array is empty.
10-10: LGTM: Removed namespace flag from cluster-scoped command.The
--namespaceflag has been correctly removed from thekubectl get nodescommand, as node resources are cluster-scoped.
15-15: LGTM: Handles both LoadBalancer IP and hostname.The JSONPath now concatenates both
.ipand.hostnamefields, ensuring the command works across cloud providers (AWS/Azure use hostname, GCP uses IP).
30-30: LGTM: Root password no longer exposed in plain text.The root password is now referenced by its secret name instead of being displayed, which prevents exposure in logs, terminal history, and CI/CD systems.
deployments/helm-chart/templates/deployment.yaml (2)
1-127: LGTM: Well-structured deployment with proper secret and config handling.The deployment template correctly:
- Uses helper templates for consistent naming and labeling
- Conditionally mounts config volumes when enabled
- References database passwords from secrets
- Implements proper health probes with reasonable timings
- Handles internal vs external database configuration
- Supports both private and public gateway modes via conditional env vars
71-87: Verify behavior when neither privateToken nor fcmCredentials is set.If both
gateway.privateTokenandgateway.fcmCredentialsare empty, theGATEWAY__MODEenvironment variable won't be set. Confirm this is intentional (e.g., the application has a default mode) or if validation is needed to require one of these values.scripts/test-helm-minikube.sh (3)
10-10: LGTM: jq added to required commands check.The
jqcommand is now verified at startup, preventing runtime failures when it's used at line 72.
27-27: LGTM: Command substitution properly quoted.The
evalcommand now properly quotes the command substitution to prevent word splitting issues.
57-68: LGTM: Robust port-forward readiness check.The polling loop with a 30-second timeout and proper cleanup on failure ensures the script doesn't proceed until the port-forward is ready or exits cleanly if it fails.
deployments/helm-chart/templates/_helpers.tpl (1)
1-42: LGTM: Standard Helm helper templates following best practices.The helper templates correctly implement:
- Name generation with override support and truncation to 63 characters
- Chart version formatting with proper character replacement
- Standard Kubernetes recommended labels
- Service account name resolution with sensible defaults
These templates align with Helm chart conventions and will be consistently used across all chart resources.
capcom6
force-pushed
the
deploy/helm-chart
branch
from
0e0feb9 to
edd21e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
deployments/helm-chart/templates/configmap.yaml (1)
1-10: Enforce non-empty inline config when mounting is enabled.Without a
requiredguard, settinggateway.config.enabled=truewhile leavinggateway.config.dataempty produces an empty ConfigMap, causing the gateway to start with a blank config. Please fail fast by wrappinggateway.config.datainrequired(or similar validation) before rendering.
Based on learningsdeployments/helm-chart/Chart.yaml (1)
5-7: RestoreappVersion: "latest"to keep CI auto-bumps working.Our release automation expects the placeholder
"latest"and will break if we hardcode"1.29.3". Please revert this value.
Based on learningsdeployments/helm-chart/templates/database.yaml (1)
46-61: Replace MariaDB probes that call missinghealthcheck.sh.The official
mariadbimage does not shiphealthcheck.sh, so these exec probes will fail and keep the pod unready. Switch the readiness/liveness probes to a supported check (e.g.,tcpSocketon 3306 ormysqladmin ping).deployments/helm-chart/templates/tests/test-connection.yaml (1)
13-14: Pin the curl test image to a specific version/digest.
curlimages/curl:latestdrifts and can pull unvetted images. Please lock this to an explicit tag (or digest) for reproducibility and supply-chain safety.deployments/helm-chart/templates/hpa.yaml (1)
1-28: Make HPA apiVersion conditional (v2 vs v2beta2) to avoid install failures on K8s 1.19–1.22Using autoscaling/v2 unconditionally breaks on clusters lacking that API. Gate on Capabilities.APIVersions and render the correct spec for v2beta2 (targetAverageUtilization fields).
Apply:
-{{- if .Values.autoscaling.enabled }} -apiVersion: autoscaling/v2 +{{- if .Values.autoscaling.enabled }} +{{- if .Capabilities.APIVersions.Has "autoscaling/v2" }} +apiVersion: autoscaling/v2 kind: HorizontalPodAutoscaler @@ metrics: - type: Resource resource: name: cpu target: type: Utilization averageUtilization: {{ .Values.autoscaling.targetCPUUtilizationPercentage }} - type: Resource resource: name: memory target: type: Utilization averageUtilization: {{ .Values.autoscaling.targetMemoryUtilizationPercentage }} -{{- end }} +{{- else }} +apiVersion: autoscaling/v2beta2 +kind: HorizontalPodAutoscaler +metadata: + name: {{ include "sms-gateway.fullname" . }} + labels: + {{- include "sms-gateway.labels" . | nindent 4 }} +spec: + scaleTargetRef: + apiVersion: apps/v1 + kind: Deployment + name: {{ include "sms-gateway.fullname" . }} + minReplicas: {{ .Values.autoscaling.minReplicas }} + maxReplicas: {{ .Values.autoscaling.maxReplicas }} + metrics: + - type: Resource + resource: + name: cpu + targetAverageUtilization: {{ .Values.autoscaling.targetCPUUtilizationPercentage }} + - type: Resource + resource: + name: memory + targetAverageUtilization: {{ .Values.autoscaling.targetMemoryUtilizationPercentage }} +{{- end }} +{{- end }}If you only support >=1.23, update README accordingly instead.
🧹 Nitpick comments (1)
deployments/helm-chart/templates/hpa.yaml (1)
15-27: Allow memory metric to be optional and support custom annotationsSome clusters prefer starting with CPU only; also exposing annotations is a common chart knob.
metadata: name: {{ include "sms-gateway.fullname" . }} labels: {{- include "sms-gateway.labels" . | nindent 4 }} + {{- with .Values.autoscaling.annotations }} + annotations: + {{- toYaml . | nindent 4 }} + {{- end }} @@ metrics: - type: Resource resource: name: cpu target: type: Utilization averageUtilization: {{ .Values.autoscaling.targetCPUUtilizationPercentage }} - - type: Resource - resource: - name: memory - target: - type: Utilization - averageUtilization: {{ .Values.autoscaling.targetMemoryUtilizationPercentage }} + {{- if .Values.autoscaling.targetMemoryUtilizationPercentage }} + - type: Resource + resource: + name: memory + target: + type: Utilization + averageUtilization: {{ .Values.autoscaling.targetMemoryUtilizationPercentage }} + {{- end }}Mirror the conditional for the v2beta2 branch using targetAverageUtilization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.github/workflows/publish-helm-chart.yml(1 hunks)deployments/helm-chart/Chart.yaml(1 hunks)deployments/helm-chart/README.md(1 hunks)deployments/helm-chart/templates/NOTES.txt(1 hunks)deployments/helm-chart/templates/_helpers.tpl(1 hunks)deployments/helm-chart/templates/configmap.yaml(1 hunks)deployments/helm-chart/templates/database.yaml(1 hunks)deployments/helm-chart/templates/deployment.yaml(1 hunks)deployments/helm-chart/templates/hpa.yaml(1 hunks)deployments/helm-chart/templates/ingress.yaml(1 hunks)deployments/helm-chart/templates/secrets.yaml(1 hunks)deployments/helm-chart/templates/service.yaml(1 hunks)deployments/helm-chart/templates/serviceaccount.yaml(1 hunks)deployments/helm-chart/templates/tests/test-connection.yaml(1 hunks)deployments/helm-chart/values.yaml(1 hunks)scripts/test-helm-minikube.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/publish-helm-chart.yml
- scripts/test-helm-minikube.sh
- deployments/helm-chart/templates/_helpers.tpl
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-05T08:37:28.077Z
Learnt from: eznix86
PR: android-sms-gateway/server#172
File: deployments/kubernetes/sms-gateway-server/Chart.yaml:9-10
Timestamp: 2025-09-05T08:37:28.077Z
Learning: In the android-sms-gateway/server project, the Helm chart's appVersion in Chart.yaml is set to "latest" as a placeholder because CI automatically updates it with the actual version from git tags during the release process via the bump-chart-app-version.yaml workflow.
Applied to files:
deployments/helm-chart/Chart.yamldeployments/helm-chart/values.yamldeployments/helm-chart/README.md
🪛 LanguageTool
deployments/helm-chart/README.md
[grammar] ~7-~7: There might be a mistake here.
Context: ...s. ## Prerequisites - Kubernetes 1.23+ - Helm 3.2.0+ - PV provisioner support in ...
(QB_NEW_EN)
[grammar] ~8-~8: There might be a mistake here.
Context: ...isites - Kubernetes 1.23+ - Helm 3.2.0+ - PV provisioner support in the underlying...
(QB_NEW_EN)
deployments/helm-chart/templates/NOTES.txt
[grammar] ~19-~19: There might be a mistake here.
Context: ....containers[0].ports[0].containerPort}") echo "Visit http://127.0.0.1:8080 to use...
(QB_NEW_EN)
[grammar] ~21-~21: There might be a mistake here.
Context: ...espace }} $POD_NAME 8080:$CONTAINER_PORT {{- end }} 2. Check the application hea...
(QB_NEW_EN)
[grammar] ~24-~24: There might be a mistake here.
Context: ...end }} 2. Check the application health: curl http://
(QB_NEW_EN)
🪛 YAMLlint (1.37.1)
deployments/helm-chart/templates/ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/serviceaccount.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/hpa.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/configmap.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/deployment.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/secrets.yaml
[error] 4-4: syntax error: expected , but found ''
(syntax)
deployments/helm-chart/templates/database.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/service.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
deployments/helm-chart/templates/tests/test-connection.yaml
[error] 4-4: syntax error: expected , but found ''
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E
- GitHub Check: Analyze (go)
Summary by CodeRabbit