[feat] add helm chart for kubernetes #172
Conversation
eznix86
changed the title
WalkthroughAdds a new Helm chart for sms-gateway-server (chart metadata, values, examples, templates, helpers, validation, NOTES, README) and a GitHub Actions workflow to automatically build and release Helm charts on pushes to master that touch Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub
participant GA as GitHub Actions
participant Repo as Repository
participant CR as chart-releaser
Dev->>GH: Push to master (changes under deployments/kubernetes/**)
GH-->>GA: Trigger "Release Charts" workflow
GA->>Repo: actions/checkout@v4 (fetch-depth: 0)
GA->>GA: Configure git user/email
GA->>GA: Install yq
GA->>Repo: Determine last Git tag (fork-safe)
alt Tag exists
GA->>Repo: Update Chart.yaml appVersion (strip leading 'v')
end
GA->>CR: Run helm/[email protected] with CR_TOKEN
CR-->>GH: Publish Helm chart release
sequenceDiagram
autonumber
actor Op as Operator
participant Helm as Helm
participant K8s as Kubernetes API
participant Sec as Secret
participant CM as ConfigMap
participant Dep as Deployment
participant Svc as Service
participant Ing as Ingress
participant HPA as HPA
Op->>Helm: helm upgrade --install sms-gateway-server
Helm->>Helm: Validate config.secret.enabled XOR config.unsecure.enabled
alt config.secret.enabled
Helm->>Sec: Reference Secret for config
else config.unsecure.enabled
Helm->>K8s: Create ConfigMap (config.yml)
K8s-->>CM: ConfigMap created
end
Helm->>K8s: Apply Deployment and Service
opt ingress.enabled
Helm->>K8s: Apply Ingress
end
opt autoscaling.enabled
Helm->>K8s: Apply HPA (autoscaling/v2)
end
K8s-->>Op: Resources ready / NOTES displayed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (25)
.github/workflows/build-chart.yaml (3)
6-6: Minimize token scopeAvoid broad write-all. Restrict to contents: write (chart-releaser needs this).
-permissions: write-all +permissions: + contents: write
3-5: Tighten triggers (reduce noise) and support manual runsLimit to master and chart path; add workflow_dispatch.
on: - push: + push: + branches: + - master + paths: + - 'deployments/kubernetes/**' + workflow_dispatch:
22-28: Avoid re-uploading existing releases and add EOF newlineSet CR_SKIP_EXISTING to true; also ensure a newline at EOF to satisfy yamllint.
- name: Run chart-releaser id: cr uses: helm/[email protected] env: CR_TOKEN: "${{ secrets.GITHUB_TOKEN }}" + CR_SKIP_EXISTING: "true" with: charts_dir: ./deployments/kubernetes +deployments/kubernetes/sms-gateway-server/examples/values.yaml (2)
21-23: Clarify Secret contentDocument that the Secret must contain a key named config.yml for the mounted file.
secret: enabled: true - secretName: "sms-gateway-config" + secretName: "sms-gateway-config" # Secret must contain key: config.yml
31-31: Add newline at EOFFixes yamllint warning.
- targetCPUUtilizationPercentage: 80 + targetCPUUtilizationPercentage: 80 +deployments/kubernetes/sms-gateway-server/Chart.yaml (1)
7-9: Verify appVersion and consider enriching metadata
- Confirm appVersion "1.28.1" exists as a published image tag and matches the server release you intend to deploy; the Deployment defaults to .Chart.AppVersion when image.tag is empty.
- Optional: add kubeVersion, sources, keywords, and maintainers for better discoverability (e.g., Artifact Hub).
version: 0.2.0 - appVersion: "1.28.1" +keywords: + - sms + - gateway + - messaging +sources: + - https://github.com/android-sms-gateway/server +maintainers: + - name: android-sms-gateway + email: [email protected] +# Optional: constrain supported Kubernetes versions +# kubeVersion: ">=1.23.0"deployments/kubernetes/sms-gateway-server/values.yaml (5)
136-138: Swap environment variable hints for FCM settingsTimeout and debounce annotations are inverted.
- timeout_seconds: 1 # push notification send timeout [FCM__DEBOUNCE_SECONDS] - debounce_seconds: 5 # push notification debounce (>= 5s) [FCM__TIMEOUT_SECONDS] + timeout_seconds: 1 # push notification send timeout [FCM__TIMEOUT_SECONDS] + debounce_seconds: 5 # push notification debounce (>= 5s) [FCM__DEBOUNCE_SECONDS]
110-117: Mutual exclusivity: config.secret vs config.unsecureUsers can enable both, which will conflict at volume definition time. Document exclusivity and add a template/schema validation in a follow-up (fail if both true).
72-81: Probe paths/port assumptionsEnsure the app exposes GET /health on the named http port. If different, expose values for path/port in values.yaml.
25-35: Consider sane security defaultsIf compatible with the container image, enable runAsNonRoot and readOnlyRootFilesystem by default.
-securityContext: {} +securityContext: + readOnlyRootFilesystem: true + runAsNonRoot: true +# runAsUser: 1000 +# capabilities: +# drop: [ "ALL" ]
125-135: Sensitive defaults in example configThe inline DB credentials are placeholders but appear in the chart by default. Consider neutering them (empty) to discourage accidental use; prefer Secret-based configuration.
- user: root # database user [DATABASE__USER] - password: root # database password [DATABASE__PASSWORD] + user: "" # database user [DATABASE__USER] + password: "" # database password [DATABASE__PASSWORD]deployments/kubernetes/sms-gateway-server/templates/service.yaml (2)
10-13: Align Service targetPort/name with Deployment; parameterize to avoid mismatchesIf the container port isn’t named "http" in the Deployment, this Service won’t resolve. Recommend parameterizing and defaulting to the numeric port.
ports: - - port: {{ .Values.service.port }} - targetPort: http - protocol: TCP - name: http + - port: {{ .Values.service.port }} + targetPort: {{ .Values.service.targetPort | default .Values.service.port }} + protocol: TCP + name: {{ .Values.service.portName | default "http" }}
1-15: Ignore YAMLlint on template braces; rely on helm lint or configure yamllint to skip templatesThe “expected the node content, but found '-'” is a false positive from YAMLlint parsing Helm delimiters. Use helm lint/chart-testing, or exclude templates via a repo-level .yamllint config.
deployments/kubernetes/sms-gateway-server/templates/config.yaml (2)
7-8: Trim whitespace to avoid a leading blank line in the embedded configUsing nindent with preceding spaces emits an empty first line in config.yml. Remove leading spaces and use whitespace control.
config.yml: | - {{ .Values.config.unsecure.configYml | nindent 4 }} +{{- .Values.config.unsecure.configYml | nindent 4 }}
1-9: Add rollout-on-change mechanism (checksum annotation) so pods restart on ConfigMap changesWithout a checksum annotation on the Deployment pod template, updates to this ConfigMap won’t trigger a rollout.
Follow-up (in deployment.yaml podTemplate.metadata.annotations):
checksum/config: {{ include (print $.Template.BasePath "/config.yaml") . | sha256sum }}I can provide a ready-to-apply patch to deployment.yaml if you want.
deployments/kubernetes/sms-gateway-server/templates/hpa.yaml (2)
2-2: Optional: apiVersion gating for broader cluster compatibilityIf you need to support older clusters, gate autoscaling/v2 via Capabilities. Otherwise, current usage is fine.
-apiVersion: autoscaling/v2 +apiVersion: {{- if .Capabilities.APIVersions.Has "autoscaling/v2" -}}autoscaling/v2{{- else -}}autoscaling/v2beta2{{- end }}
1-1: YAMLlint false positive on Helm control structuresSame as other templates; prefer helm lint or configure YAMLlint to ignore templated files.
deployments/kubernetes/sms-gateway-server/templates/NOTES.txt (2)
8-11: NodePort URL discovery can pick a non-routable node; prefer ExternalIP fallbackUsing the first node’s address can mislead on multi-node clusters. Suggest trying an ExternalIP first, then InternalIP/minikube ip.
- export NODE_IP=$(kubectl get nodes --namespace {{ .Release.Namespace }} -o jsonpath="{.items[0].status.addresses[0].address}") + export NODE_IP=$(kubectl get nodes -o jsonpath='{.items[0].status.addresses[?(@.type=="ExternalIP")].address}') + if [ -z "$NODE_IP" ]; then NODE_IP=$(kubectl get nodes -o jsonpath='{.items[0].status.addresses[?(@.type=="InternalIP")].address}'); fi
3-7: HTTPS scheme detection may be inaccurate per-hostUsing .Values.ingress.tls to decide “http” vs “https” is global. A host without TLS will still print https if any TLS is configured. Consider computing scheme per host.
deployments/kubernetes/sms-gateway-server/templates/ingress.yaml (1)
1-1: Ignore YAMLlint on Helm delimiters; use helm lint/chart-testing insteadSame false-positive pattern as other files.
deployments/kubernetes/sms-gateway-server/templates/deployment.yaml (4)
30-39: Wire service account into the Pod specYou defined a
serviceAccountNamehelper but it’s not used here. Set it explicitly so RBAC works as intended and to avoid running under the default SA.Apply this diff:
{{- with .Values.podSecurityContext }} securityContext: {{- toYaml . | nindent 8 }} {{- end }} + serviceAccountName: {{ include "sms-gateway-server.serviceAccountName" . }} containers:
49-52: Decouple containerPort from Service port
containerPortshould reflect the app’s listening port, not the Service’s external port. Couple this to a dedicated value (e.g.,containerPort) and have the Service target that port by name.Apply this diff:
- containerPort: {{ .Values.service.port }} + containerPort: {{ .Values.containerPort | default 8080 }}Follow-up: ensure
templates/service.yamlusestargetPort: http(by name) ortargetPort: {{ .Values.containerPort | default 8080 }}.
16-19: Add checksum annotation to trigger rollout on config changesWithout a config checksum on the Pod template, changing the Secret/ConfigMap won’t roll pods automatically.
Apply this diff:
- {{- with .Values.podAnnotations }} - annotations: - {{- toYaml . | nindent 8 }} - {{- end }} + annotations: + checksum/config: {{ include (print $.Template.BasePath "/config.yaml") . | sha256sum }} + {{- with .Values.podAnnotations }} + {{- toYaml . | nindent 8 }} + {{- end }}
65-66: Remove trailing whitespace flagged by linterMinor cleanup; trims the trailing spaces on the blank line.
Apply this diff:
- +deployments/kubernetes/sms-gateway-server/templates/_helpers.tpl (1)
36-43: Add standard part-of/component labels for better discoverabilityOptional labels improve grouping in UIs and policies.
Apply this diff:
{{- define "sms-gateway-server.labels" -}} helm.sh/chart: {{ include "sms-gateway-server.chart" . }} {{ include "sms-gateway-server.selectorLabels" . }} {{- if .Chart.AppVersion }} app.kubernetes.io/version: {{ .Chart.AppVersion | quote }} {{- end }} app.kubernetes.io/managed-by: {{ .Release.Service }} +app.kubernetes.io/part-of: {{ include "sms-gateway-server.name" . }} +app.kubernetes.io/component: server {{- end }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
.github/workflows/build-chart.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/.helmignore(1 hunks)deployments/kubernetes/sms-gateway-server/Chart.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/examples/values.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/templates/NOTES.txt(1 hunks)deployments/kubernetes/sms-gateway-server/templates/_helpers.tpl(1 hunks)deployments/kubernetes/sms-gateway-server/templates/config.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/templates/deployment.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/templates/hpa.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/templates/ingress.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/templates/service.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/values.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
deployments/kubernetes/sms-gateway-server/templates/service.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
deployments/kubernetes/sms-gateway-server/templates/config.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/kubernetes/sms-gateway-server/templates/hpa.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/kubernetes/sms-gateway-server/templates/ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
.github/workflows/build-chart.yaml
[error] 28-28: no new line character at the end of file
(new-line-at-end-of-file)
deployments/kubernetes/sms-gateway-server/examples/values.yaml
[error] 31-31: no new line character at the end of file
(new-line-at-end-of-file)
deployments/kubernetes/sms-gateway-server/templates/deployment.yaml
[warning] 9-9: wrong indentation: expected 0 but found 2
(indentation)
[error] 65-65: trailing spaces
(trailing-spaces)
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
🪛 LanguageTool
deployments/kubernetes/sms-gateway-server/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)
🪛 actionlint (1.7.7)
.github/workflows/build-chart.yaml
13-13: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (6)
deployments/kubernetes/sms-gateway-server/.helmignore (1)
1-23: LGTM: sensible defaults for Helm packagingCovers the usual OS/VCS/IDE clutter. No blockers.
deployments/kubernetes/sms-gateway-server/examples/values.yaml (1)
15-17: Port alignment with defaultsExample uses service.port 8080. Make sure the chart’s default service.port and app listen port match (see suggested default change to 8080 in values.yaml).
deployments/kubernetes/sms-gateway-server/Chart.yaml (1)
1-3: LGTM: basic chart metadata presentapiVersion v2, name, and description look fine.
deployments/kubernetes/sms-gateway-server/templates/deployment.yaml (2)
1-14: Overall: template structure looks solidGood use of helpers, autoscaling gating for replicas, optional probes/resources, and extensibility hooks (labels/annotations, nodeSelector/affinity/tolerations).
1-11: Manual Verification Required: Helm binary unavailable in CI sandboxI attempted to render the Helm chart and lint the output, but the
helmcommand isn’t installed in this environment, so I can’t automatically confirm that the YAMLlint warnings are false positives. Please manually render and lint the chart to ensure the Go template directives aren’t causing real syntax or indentation errors.• Files & lines to verify
–deployments/kubernetes/sms-gateway-server/templates/deployment.yamllines 1–11, 53–60, 73–83
• Steps to reproduce
- Run
helm template demo deployments/kubernetes/sms-gateway-server --set image.repository=dummy --set image.tag=latest > rendered.yaml- Lint with
yamllint -d "{extends: default, rules: {document-start: disable}}" rendered.yaml- If using both
config.secretandconfig.unsecure, render and search for duplicateconfig-volumeentries to catch volume collisions.Please confirm whether the warnings are indeed Helm templating artifacts or if adjustments are needed.
deployments/kubernetes/sms-gateway-server/templates/_helpers.tpl (1)
1-63: Helpers look good and align with Helm best practicesNaming, truncation, and selector/label composition mirror the official chart patterns. No blockers.
deployments/kubernetes/sms-gateway-server/templates/deployment.yaml
Outdated
Show resolved
Hide resolved
deployments/kubernetes/sms-gateway-server/templates/ingress.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
deployments/kubernetes/sms-gateway-server/values.yaml (1)
41-41: Port alignment fixed (matches app listen 8080).This resolves the earlier mismatch and should make OOTB routing work.
🧹 Nitpick comments (25)
deployments/kubernetes/sms-gateway-server/.helmignore (1)
1-23: Add common Helm build artifacts to ignore list.Helm typically excludes chart artifacts to keep packages lean.
.svn/ # Common backup files *.swp *.bak *.tmp *.orig *~ # Various IDEs .project .idea/ *.tmproj .vscode/ + +# Helm build artifacts +charts/ +Chart.lock +*.tgz +.helmignore + +# Local env files +.envdeployments/kubernetes/sms-gateway-server/examples/values.yaml (3)
3-6: Consider pinning image tag and relax pullPolicy.Explicit tag improves reproducibility; If tag is pinned, IfNotPresent is usually preferred.
image: repository: capcom6/sms-gateway - pullPolicy: Always + # tag: "{{ .Chart.AppVersion }}" # uncomment if templates default to Chart.AppVersion + pullPolicy: IfNotPresent
19-26: Avoid embedding secrets in values; prefer Secret-based config.Good that unsecure is disabled by default and secret-based config is present. Keep sensitive data out of values files (they often land in VCS).
27-31: Add EOF newline.YAML lint flagged missing trailing newline.
maxReplicas: 100 targetCPUUtilizationPercentage: 80 +deployments/kubernetes/sms-gateway-server/Chart.yaml (1)
7-9: Metadata enhancements (optional).Consider adding home, sources, maintainers, and kubeVersion for better cataloging (e.g., Artifact Hub).
version: 0.2.6 appVersion: "1.28.1" +home: https://github.com/android-sms-gateway/server +sources: + - https://github.com/android-sms-gateway/server +maintainers: + - name: android-sms-gateway maintainers + email: [email protected] +# kubeVersion: ">=1.24.0-0"deployments/kubernetes/sms-gateway-server/templates/config.yaml (3)
4-6: Add standard labels to ConfigMap for consistency.Align with other resources using chart helpers.
metadata: name: {{ include "sms-gateway-server.fullname" . }}-config + labels: + {{- include "sms-gateway-server.labels" . | nindent 4 }}
7-8: Avoid double-indentation in block literal.Since nindent adds indentation, drop the leading spaces before the template action for cleaner output (functional either way).
config.yml: | - {{ .Values.config.unsecure.configYml | nindent 4 }} +{{ .Values.config.unsecure.configYml | nindent 4 }}
1-9: Rollout on config change (ensure Deployment uses checksum).Make sure Deployment adds an annotation with a checksum of this ConfigMap to trigger pod restarts on config updates.
I can propose the checksum snippet for deployment if needed.
.github/workflows/build-chart.yaml (3)
6-6: Tighten permissions to least privilege
write-allis overly broad.chart-releaserneedscontents: writeto push gh-pages and releases.Apply:
-permissions: write-all +permissions: + contents: write
3-5: Scope workflow to relevant branches/pathsAvoid running on unrelated pushes.
Apply:
on: push: + branches: [ master ] + paths: + - 'deployments/kubernetes/**' + - '.github/workflows/build-chart.yaml'
29-29: Add newline at EOFSatisfy yamllint “no new line at end of file”.
deployments/kubernetes/sms-gateway-server/templates/NOTES.txt (3)
8-17: Use strict equality for service.type comparisons
containscan misfire; compare exact types.Apply:
-{{- else if contains "NodePort" .Values.service.type }} +{{- else if eq .Values.service.type "NodePort" }} ... -{{- else if contains "LoadBalancer" .Values.service.type }} +{{- else if eq .Values.service.type "LoadBalancer" }} ... -{{- else if contains "ClusterIP" .Values.service.type }} +{{- else if eq .Values.service.type "ClusterIP" }}
10-10: Remove namespace flag for cluster-scoped nodes
kubectl get nodesignores namespaces; drop it to avoid confusion.Apply:
- export NODE_IP=$(kubectl get nodes --namespace {{ .Release.Namespace }} -o jsonpath="{.items[0].status.addresses[0].address}") + export NODE_IP=$(kubectl get nodes -o jsonpath="{.items[0].status.addresses[0].address}")
18-21: Prefer selecting a Running pod for port-forwardCurrent jsonpath may choose a terminating pod.
Apply:
- export POD_NAME=$(kubectl get pods --namespace {{ .Release.Namespace }} -l "app.kubernetes.io/name={{ include "sms-gateway-server.name" . }},app.kubernetes.io/instance={{ .Release.Name }}" -o jsonpath="{.items[0].metadata.name}") + export POD_NAME=$(kubectl get pods --namespace {{ .Release.Namespace }} -l "app.kubernetes.io/name={{ include "sms-gateway-server.name" . }},app.kubernetes.io/instance={{ .Release.Name }}" \ + -o jsonpath='{.items[?(@.status.phase=="Running")].metadata.name}' | awk '{print $1}')deployments/kubernetes/sms-gateway-server/templates/ingress.yaml (2)
23-23: Quote secretName for safetyAvoid parsing issues with special chars.
Apply:
- secretName: {{ .secretName }} + secretName: {{ .secretName | quote }}
32-33: Quote paths and default to "/" when unsetPrevents invalid manifests if a path is omitted or contains special chars.
Apply:
- - path: {{ .path }} + - path: {{ .path | default "/" | quote }} pathType: {{ .pathType | default "ImplementationSpecific" }}deployments/kubernetes/sms-gateway-server/templates/hpa.yaml (1)
13-14: Validate minReplicas <= maxReplicas at template-timeFail fast on misconfiguration.
Apply:
spec: + {{- if gt .Values.autoscaling.minReplicas .Values.autoscaling.maxReplicas }} + {{- fail "autoscaling.minReplicas cannot be greater than autoscaling.maxReplicas" }} + {{- end }} scaleTargetRef:deployments/kubernetes/sms-gateway-server/templates/deployment.yaml (2)
25-33: Expose serviceAccountName via helperEnables RBAC-friendly customization without editing the template.
Apply:
spec: + serviceAccountName: {{ include "sms-gateway-server.serviceAccountName" . }} {{- with .Values.imagePullSecrets }} imagePullSecrets:
65-65: Remove trailing whitespaceMinor lint clean-up.
- +deployments/kubernetes/sms-gateway-server/values.yaml (5)
73-80: Probe port depends on a named containerPort "http". Verify or switch to numeric 8080.If the container doesn’t expose a port named "http", probes will fail. Either ensure the Deployment names containerPort "http", or use the numeric port.
Proposed change (numeric port):
livenessProbe: httpGet: path: /health - port: http + port: 8080 readinessProbe: httpGet: path: /health - port: http + port: 8080
118-119: Avoid shipping a real-looking default private token.Use a clear placeholder to prevent accidental exposure in dev clusters.
- private_token: 123456789 # access token for device registration in private mode [GATEWAY__PRIVATE_TOKEN] + private_token: "CHANGE_ME" # access token for device registration in private mode [GATEWAY__PRIVATE_TOKEN]
122-124: Default trusted proxy should be empty.Trusting 127.0.0.1 rarely helps in-cluster and can confuse users behind Ingress/LB. Let users set their own CIDRs/IPs.
- proxies: - - "127.0.0.1" # proxy address [HTTP__PROXIES] + proxies: [] # trusted proxies (CIDRs or IPs). Set if behind a known proxy. [HTTP__PROXIES]
125-131: Use non-root placeholders and avoid localhost for DB defaults."localhost" won’t work in-cluster; encourage service DNS and safer creds to reduce footguns when toggling unsecure config.
- host: localhost # database host [DATABASE__HOST] + host: "mysql" # database host/service DNS [DATABASE__HOST] - user: root # database user [DATABASE__USER] - password: root # database password [DATABASE__PASSWORD] + user: "sms" # database user [DATABASE__USER] + password: "CHANGE_ME" # database password [DATABASE__PASSWORD]
16-17: Expose serviceAccount settings in values to match helpers._helpers.tpl references .Values.serviceAccount.* but defaults aren’t present here. Add them so users discover the knobs.
fullnameOverride: "" +serviceAccount: + create: true + automount: true + annotations: {} + name: "" + # This is for setting Kubernetes Annotations to a Pod.deployments/kubernetes/sms-gateway-server/templates/_helpers.tpl (1)
36-43: Consider adding conventional labels for better grouping.Optional, but app.kubernetes.io/part-of and app.kubernetes.io/component help UIs/grouping.
{{- define "sms-gateway-server.labels" -}} helm.sh/chart: {{ include "sms-gateway-server.chart" . }} {{ include "sms-gateway-server.selectorLabels" . }} {{- if .Chart.AppVersion }} app.kubernetes.io/version: {{ .Chart.AppVersion | quote }} {{- end }} app.kubernetes.io/managed-by: {{ .Release.Service }} +app.kubernetes.io/part-of: sms-gateway +app.kubernetes.io/component: server {{- end }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
.github/workflows/build-chart.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/.helmignore(1 hunks)deployments/kubernetes/sms-gateway-server/Chart.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/examples/values.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/templates/NOTES.txt(1 hunks)deployments/kubernetes/sms-gateway-server/templates/_helpers.tpl(1 hunks)deployments/kubernetes/sms-gateway-server/templates/config.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/templates/deployment.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/templates/hpa.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/templates/ingress.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/templates/service.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/values.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
deployments/kubernetes/sms-gateway-server/templates/config.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
.github/workflows/build-chart.yaml
[error] 29-29: no new line character at the end of file
(new-line-at-end-of-file)
deployments/kubernetes/sms-gateway-server/examples/values.yaml
[error] 31-31: no new line character at the end of file
(new-line-at-end-of-file)
deployments/kubernetes/sms-gateway-server/templates/service.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
deployments/kubernetes/sms-gateway-server/templates/ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/kubernetes/sms-gateway-server/templates/deployment.yaml
[warning] 9-9: wrong indentation: expected 0 but found 2
(indentation)
[error] 65-65: trailing spaces
(trailing-spaces)
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
deployments/kubernetes/sms-gateway-server/templates/hpa.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🪛 actionlint (1.7.7)
.github/workflows/build-chart.yaml
12-12: step must run script with "run" section or run action with "uses" section
(syntax-check)
🪛 LanguageTool
deployments/kubernetes/sms-gateway-server/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)
🔇 Additional comments (5)
deployments/kubernetes/sms-gateway-server/examples/values.yaml (1)
15-17: Verify service port aligns with container port name.Service uses targetPort: http in templates; ensure the containerPort in Deployment is named "http".
deployments/kubernetes/sms-gateway-server/templates/service.yaml (2)
10-13: Confirm targetPort name matches containerPort.targetPort: http assumes the container port is named "http" in Deployment; otherwise Service won’t route.
5-6: YAML linter false-positive — Go template trim markerThe hyphen in
labels: {{- include "sms-gateway-server.labels" . | nindent 4 }}is a Go template whitespace-trim marker, not a YAML list dash. The final rendered YAML under
metadata.labelsis valid. Please usehelm lintorhelm template(with your values file) to validate this template instead of a plain YAML linter.Affected file:
- deployments/kubernetes/sms-gateway-server/templates/service.yaml (lines 5–6)
deployments/kubernetes/sms-gateway-server/templates/config.yaml (1)
1-1: YAML linter false-positive on leading trim marker.{{- if ... }} is valid Helm templating. Use helm template/lint to validate rendered manifests.
deployments/kubernetes/sms-gateway-server/templates/_helpers.tpl (1)
36-43: Helpers and labels look standard and solid.Naming, truncation, and label set follow Helm best practices.
deployments/kubernetes/sms-gateway-server/templates/deployment.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Hello!
I really appreciate your contribution. I hadn't planned to deploy the app to Kubernetes, and I'm not entirely sure it will work as expected with more than one replica due to some current architecture decisions. However, having a Helm chart will definitely be useful for the future.
I've left some comments and questions on the files—could you address them? Please also review the nitpick comments from CodeRabbit. Unfortunately, I don't have enough expertise in Kubernetes and Helm to review your PR in detail.
|
One more thing. What about the optional MariaDB deployment? So the Helm chart will be self-contained? Something like: |
|
Yeah I think it is a good idea to have an optional database in it ! |
|
I managed to deploy it on my side, and I have a few questions:
What do you think? |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/bump-chart-app-version.yaml (1)
25-46: Fix CHART_FILE path in bump workflow
In .github/workflows/bump-chart-app-version.yaml (line 28), change:
- CHART_FILE=deployments/kubernetes/Chart.yaml
- CHART_FILE=deployments/kubernetes/sms-gateway-server/Chart.yaml
♻️ Duplicate comments (3)
deployments/kubernetes/sms-gateway-server/examples/values.yaml (2)
3-5: Switch image to GHCR (per prior feedback).Use ghcr.io/android-sms-gateway/server instead of capcom6/sms-gateway in examples.
image: - repository: capcom6/sms-gateway + repository: ghcr.io/android-sms-gateway/server pullPolicy: Always
27-31: Disable HPA in example to avoid unintended multi-replica scaling.Project isn’t optimized for >1 replicas; keep autoscaling off (or hard-cap to 1).
autoscaling: - enabled: true - minReplicas: 1 - maxReplicas: 100 - targetCPUUtilizationPercentage: 80 + enabled: false + minReplicas: 1 + maxReplicas: 1 + targetCPUUtilizationPercentage: 80deployments/kubernetes/sms-gateway-server/templates/config.yaml (1)
6-7: Fill labels with the chart’s common labels (or remove the empty block).Empty
labels:yields invalid metadata; use the helper as previously discussed.metadata: name: {{ include "sms-gateway-server.fullname" . }}-config - labels: - # ... + labels: + {{- include "sms-gateway-server.labels" . | nindent 4 }}
🧹 Nitpick comments (13)
deployments/kubernetes/sms-gateway-server/examples/values.yaml (2)
1-1: Consider pinning image tag in examples.If the chart defaults to Chart.appVersion when tag is omitted, add a comment to make it explicit for users.
31-31: Add trailing newline.Fix YAML lint warning.
.github/workflows/build-chart.yaml (3)
1-34: Add workflow concurrency to prevent overlapping publishes.Avoid racing chart releases on rapid pushes.
name: Release Charts + +concurrency: + group: charts-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true
28-34: Consider pinning actions to commit SHAs.Improves supply-chain safety; keep the tag as a comment for readability.
34-34: Add trailing newline.Silences lint.
deployments/kubernetes/sms-gateway-server/Chart.yaml (2)
1-16: Optional chart metadata improvements.
- Add kubeVersion to document supported clusters.
- Add an icon (if available).
description: A Helm chart for Kubernetes to deploy sms-gateway-server. type: application +kubeVersion: ">=1.24.0"
16-16: Add trailing newline.Silences lint.
.github/workflows/bump-chart-app-version.yaml (2)
1-1: Rename workflow to avoid duplicate name with build pipeline.Prevents UI confusion.
-name: Release Charts +name: Bump Helm Chart Version
25-46: Optionally support multi-chart trees.If more charts are added later, iterate all Chart.yaml files under deployments/kubernetes/**.
- CHART_FILE=deployments/kubernetes/sms-gateway-server/Chart.yaml + CHART_FILES=$(fd -a 'Chart.yaml' deployments/kubernetes) ... - # Extract current version - CURRENT_VERSION=$(awk '/^version:/ {print $2}' $CHART_FILE) - IFS='.' read -r MAJOR MINOR PATCH <<< "$CURRENT_VERSION" + for CHART_FILE in $CHART_FILES; do + CURRENT_VERSION=$(awk '/^version:/ {print $2}' "$CHART_FILE") + IFS='.' read -r MAJOR MINOR PATCH <<< "$CURRENT_VERSION" # Increment patch PATCH=$((PATCH + 1)) NEW_VERSION="$MAJOR.$MINOR.$PATCH" # Update Chart.yaml - sed -i "s/^version:.*/version: $NEW_VERSION/" $CHART_FILE - sed -i "s/^appVersion:.*/appVersion: $TAG_VERSION/" $CHART_FILE + sed -i "s/^version:.*/version: $NEW_VERSION/" "$CHART_FILE" + sed -i "s/^appVersion:.*/appVersion: \"$TAG_VERSION\"/" "$CHART_FILE" - # Commit and push changes - git add $CHART_FILE + done + # Commit and push changes + git add $CHART_FILESdeployments/kubernetes/sms-gateway-server/templates/deployment.yaml (4)
53-55: Decouple containerPort from service.portAllow differing container and service ports; default to service.port to keep backwards compatibility.
Apply:
- containerPort: {{ .Values.service.port }} + containerPort: {{ .Values.containerPort | default .Values.service.port }}Also add
containerPortto values.yaml with a comment.
16-19: Add checksum annotation to trigger rollout on config changesEnsure Pods roll on ConfigMap changes (when using unsecure config).
Apply:
{{- with .Values.podAnnotations }} annotations: {{- toYaml . | nindent 8 }} {{- end }} + {{- if .Values.config.unsecure.enabled }} + annotations: + checksum/config: {{ include (print $.Template.BasePath "/config.yaml") . | sha256sum }} + {{- end }}Note: keep both annotations blocks merged if desired.
40-41: Optional: support image digest pinningPrefer immutable digests for supply-chain safety; fall back to tag if no digest provided.
- image: "{{ .Values.image.repository }}:{{ .Values.image.tag | default .Chart.AppVersion }}" + {{- if .Values.image.digest }} + image: "{{ .Values.image.repository }}@{{ .Values.image.digest }}" + {{- else }} + image: "{{ .Values.image.repository }}:{{ .Values.image.tag | default .Chart.AppVersion }}" + {{- end }}
68-68: Trim trailing whitespaceThere's trailing whitespace on this blank line; please remove to satisfy linters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
.github/workflows/build-chart.yaml(1 hunks).github/workflows/bump-chart-app-version.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/Chart.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/examples/values.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/templates/config.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/templates/deployment.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/values.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- deployments/kubernetes/sms-gateway-server/values.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/build-chart.yaml
[error] 34-34: no new line character at the end of file
(new-line-at-end-of-file)
deployments/kubernetes/sms-gateway-server/Chart.yaml
[error] 16-16: no new line character at the end of file
(new-line-at-end-of-file)
deployments/kubernetes/sms-gateway-server/examples/values.yaml
[error] 31-31: no new line character at the end of file
(new-line-at-end-of-file)
deployments/kubernetes/sms-gateway-server/templates/config.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/kubernetes/sms-gateway-server/templates/deployment.yaml
[warning] 9-9: wrong indentation: expected 0 but found 2
(indentation)
[error] 68-68: trailing spaces
(trailing-spaces)
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (1)
deployments/kubernetes/sms-gateway-server/templates/config.yaml (1)
1-1: YAML lint error is a false positive.The
{{- if ... }}directive confuses static linters; safe to ignore.
deployments/kubernetes/sms-gateway-server/templates/config.yaml
Outdated
Show resolved
Hide resolved
deployments/kubernetes/sms-gateway-server/templates/deployment.yaml
Outdated
Show resolved
Hide resolved
deployments/kubernetes/sms-gateway-server/templates/deployment.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
deployments/kubernetes/sms-gateway-server/Chart.yaml (2)
1-3: Add kubeVersion, keywords, and Artifact Hub annotations.Improves scheduling compatibility and catalog visibility.
Apply:
apiVersion: v2 name: sms-gateway-server description: A Helm chart for Kubernetes to deploy sms-gateway-server. type: application version: 0.2.9 appVersion: "1.29.0" +keywords: + - sms + - gateway + - messaging +kubeVersion: ">=1.22.0-0" +annotations: + artifacthub.io/license: MIT + artifacthub.io/category: networking + artifacthub.io/prerelease: "false"Also applies to: 5-9
16-16: Add trailing newline.Fixes YAMLlint “no new line at end of file”.
deployments/kubernetes/sms-gateway-server/templates/config.yaml (1)
1-11: Optionally allow templating inside config.yml and address lint noise.
- If you want values to be interpolated within configYml, wrap with tpl.
- Exclude Helm templates from YAMLlint to avoid false positives.
Apply:
- config.yml: | - {{ .Values.config.unsecure.configYml | nindent 4 }} + config.yml: | + {{ tpl .Values.config.unsecure.configYml $ | nindent 4 }}And in your lint config, ignore
deployments/kubernetes/**/templates/**.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.github/workflows/bump-chart-app-version.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/Chart.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/templates/_validation.tpl(1 hunks)deployments/kubernetes/sms-gateway-server/templates/config.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/templates/deployment.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/bump-chart-app-version.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
deployments/kubernetes/sms-gateway-server/Chart.yaml
[error] 16-16: no new line character at the end of file
(new-line-at-end-of-file)
deployments/kubernetes/sms-gateway-server/templates/config.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/kubernetes/sms-gateway-server/templates/deployment.yaml
[warning] 9-9: wrong indentation: expected 0 but found 2
(indentation)
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (5)
deployments/kubernetes/sms-gateway-server/Chart.yaml (2)
11-16: Good metadata addition (home/sources/maintainers).These fields address prior feedback and improve chart discoverability.
7-9: Verify automation for bumping ChartversionandappVersionNo bump-workflow steps detected under
.github/workflows—ensure your CI or Renovate setup:
- Auto-updates
appVersionon each release tag- Increments Chart
versiononly when chart templates changedeployments/kubernetes/sms-gateway-server/templates/_validation.tpl (1)
1-3: Fail-fast mutual exclusivity looks good.Clear message and early abort keep renders sane.
deployments/kubernetes/sms-gateway-server/templates/deployment.yaml (2)
42-51: Set CONFIG_PATH only when a config is mounted (or document a default).Avoids runtime failures if the app expects the file to exist.
Option A (guarded):
- env: + {{- if or .Values.config.secret.enabled .Values.config.unsecure.enabled }} + env: - name: CONFIG_PATH value: /app/config.yml {{- if .Values.config.debug }} - name: DEBUG value: "1" {{- end }} {{- with .Values.extraEnv }} {{- toYaml . | nindent 12 }} {{- end }} + {{- else }} + {{- with .Values.extraEnv }} + env: + {{- toYaml . | nindent 12 }} + {{- end }} + {{- end }}Option B: keep as-is if the binary tolerates missing
/app/config.yml.
68-75: Nice consolidation of volumeMounts and readOnly: true.This avoids duplicate mounts and enforces immutability.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
deployments/kubernetes/sms-gateway-server/Chart.yaml (1)
11-16: Good metadata addition (home/sources/maintainers).Matches earlier feedback; thanks for adding it.
deployments/kubernetes/sms-gateway-server/templates/config.yaml (1)
6-8: Labels on ConfigMap look good.Aligns with helpers; satisfies earlier feedback.
deployments/kubernetes/sms-gateway-server/templates/ingress.yaml (1)
34-34: Addressed: default pathType now always rendered.Good fix; avoids invalid Ingress on v1 API.
deployments/kubernetes/sms-gateway-server/templates/deployment.yaml (1)
81-96: Guard the entire volumes block; current template can emit an empty “volumes:” (invalid Pod).If both config sources are disabled and
.Values.volumesis empty, this renders a barevolumes:key.- volumes: - {{- if and .Values.config.secret.enabled .Values.config.unsecure.enabled }} - {{- fail "Only one of config.secret.enabled or config.unsecure.enabled may be true" }} - {{- end }} - {{- if .Values.config.secret.enabled }} - - name: config-volume - secret: - secretName: {{ required "Values.config.secret.secretName is required when config.secret.enabled is true" .Values.config.secret.secretName }} - {{- else if .Values.config.unsecure.enabled }} - - name: config-volume - configMap: - name: {{ include "sms-gateway-server.fullname" $ }}-config - {{- end }} - {{- with .Values.volumes }} - {{- toYaml . | nindent 8 }} - {{- end }} + {{- if or (or .Values.config.secret.enabled .Values.config.unsecure.enabled) .Values.volumes }} + volumes: + {{- if and .Values.config.secret.enabled .Values.config.unsecure.enabled }} + {{- fail "Only one of config.secret.enabled or config.unsecure.enabled may be true" }} + {{- end }} + {{- if .Values.config.secret.enabled }} + - name: config-volume + secret: + secretName: {{ required "Values.config.secret.secretName is required when config.secret.enabled is true" .Values.config.secret.secretName }} + {{- else if .Values.config.unsecure.enabled }} + - name: config-volume + configMap: + name: {{ include "sms-gateway-server.fullname" $ }}-config + {{- end }} + {{- with .Values.volumes }} + {{- toYaml . | nindent 8 }} + {{- end }} + {{- end }}
🧹 Nitpick comments (19)
deployments/kubernetes/sms-gateway-server/Chart.yaml (2)
1-16: Add kubeVersion, keywords, and optional Artifact Hub annotations.Helps tooling compatibility and discoverability. Safe, backward-compatible metadata additions.
Apply:
apiVersion: v2 name: sms-gateway-server description: A Helm chart for Kubernetes to deploy sms-gateway-server. type: application version: 0.2.10 appVersion: "1.29.0" home: https://github.com/android-sms-gateway/server sources: - https://github.com/android-sms-gateway/server maintainers: - name: SMSGate Team email: [email protected] +keywords: + - sms + - gateway + - notifications + - android +kubeVersion: ">=1.25.0-0" +# Optional, if you plan to publish on Artifact Hub: +annotations: + artifacthub.io/category: "communication" + artifacthub.io/links: | + - name: Source + url: https://github.com/android-sms-gateway/server
16-16: Add newline at EOF.Silences linters (new-line-at-end-of-file).
- email: [email protected] + email: [email protected] +deployments/kubernetes/sms-gateway-server/values.yaml (5)
121-121: Remove trailing whitespace.Fixes YAMLlint (trailing-spaces).
- # a config for you and it will apply what you configured. + # a config for you and it will apply what you configured.
1-1: Normalize blank lines (top and bottom).Keeps YAMLlint happy (empty-lines).
- replicaCount: 1 @@ - +Also applies to: 149-149
28-35: Harden default securityContext (opt-in).Reasonable secure defaults; users can override if needed.
securityContext: {} - # capabilities: - # drop: - # - ALL - # readOnlyRootFilesystem: true - # runAsNonRoot: true - # runAsUser: 1000 + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + runAsNonRoot: true + # If your image supports it, set an explicit UID: + # runAsUser: 1000 + capabilities: + drop: + - ALL
36-41: Expose service.annotations in values.Handy for cloud LB, service mesh, etc. Add key now to avoid schema churn later.
service: # This sets the service type more information can be found here: https://kubernetes.io/docs/concepts/services-networking/service/#publishing-services-service-types type: ClusterIP + # Optional annotations for the Service + annotations: {} # This sets the ports more information can be found here: https://kubernetes.io/docs/concepts/services-networking/service/#field-spec-ports port: 8080And render in templates/service.yaml (see related comment).
71-80: Make probe timings configurable.Expose common probe fields; defaults mirror README examples.
livenessProbe: httpGet: path: /health port: http + initialDelaySeconds: 30 + periodSeconds: 10 + timeoutSeconds: 2 + failureThreshold: 3 readinessProbe: httpGet: path: /health port: http + initialDelaySeconds: 5 + periodSeconds: 5 + timeoutSeconds: 2 + failureThreshold: 3deployments/kubernetes/sms-gateway-server/README.md (1)
170-183: Optional: Document internal DB option status.If you decide to ship an optional MariaDB (e.g., Values.database.deployInternal), add a short note here linking to the values and templates. I can draft the templates (StatefulSet/Service/Secret/PVC) behind a flag.
deployments/kubernetes/sms-gateway-server/templates/config.yaml (2)
1-1: Add newline at EOF; ignore YAMLlint “-” syntax warning.Helm directives trip YAMLlint; safe to ignore. Still add newline to quiet other linters.
{{- if .Values.config.unsecure.enabled}} @@ -{{- end }} +{{- end }} +Also applies to: 12-12
1-11: Ensure rollout on config changes (checksum annotation in Deployment).Add a checksum of config to the Pod template annotations so updates trigger rolling restarts.
In Deployment template’s pod spec metadata.annotations:
checksum/config: {{ include (print $.Template.BasePath "/config.yaml") . | sha256sum }}If config is secret-based, compute checksum from the referenced Secret data similarly.
deployments/kubernetes/sms-gateway-server/templates/service.yaml (2)
3-7: Allow service annotations.Common need for cloud LBs/meshes; wire to values.service.annotations.
metadata: name: {{ include "sms-gateway-server.fullname" . }} labels: {{- include "sms-gateway-server.labels" . | nindent 4 }} app.kubernetes.io/component: service + {{- with .Values.service.annotations }} + annotations: + {{- toYaml . | nindent 4 }} + {{- end }}
11-14: Optional: set appProtocol for better integration.Helps gateways/proxies infer protocol.
- port: {{ .Values.service.port }} targetPort: http protocol: TCP name: http + appProtocol: httpdeployments/kubernetes/sms-gateway-server/templates/ingress.yaml (2)
33-34: Default/quote path to avoid invalid empty values.If a user omits .path, render "/" and quote the string.
- - path: {{ .path }} - pathType: {{ .pathType | default "ImplementationSpecific" }} + - path: {{ .path | default "/" | quote }} + pathType: {{ .pathType | default "ImplementationSpecific" }}
24-24: Quote TLS secretName.Prevents parsing surprises with special chars; harmless otherwise.
- secretName: {{ .secretName }} + secretName: {{ .secretName | quote }}deployments/kubernetes/sms-gateway-server/templates/hpa.yaml (1)
9-15: Optional: add scaleDown stabilization to reduce flapping.Consider a conservative default behavior block.
spec: scaleTargetRef: @@ minReplicas: {{ .Values.autoscaling.minReplicas }} maxReplicas: {{ .Values.autoscaling.maxReplicas }} + behavior: + scaleDown: + stabilizationWindowSeconds: {{ .Values.autoscaling.scaleDownStabilizationSeconds | default 300 }} + policies: + - type: Percent + value: {{ .Values.autoscaling.scaleDownPercent | default 10 }} + periodSeconds: 60deployments/kubernetes/sms-gateway-server/examples/values.yaml (3)
31-31: Trim trailing whitespace.Minor formatting cleanup.
- # a config for you and it will apply what you configured. + # a config for you and it will apply what you configured.
73-73: Add a newline at EOF.Keeps linters and diffs happy.
- targetCPUUtilizationPercentage: 80 + targetCPUUtilizationPercentage: 80 +
61-67: Use neutral placeholders in examples.Avoid pointing to a real host in sample values.
- - name: HTTP__API__HOST - value: https://sms-gateway.parksmart.mu + - name: HTTP__API__HOST + value: https://example.com @@ - - name: HTTP__API__PATH - value: /api + - name: HTTP__API__PATH + value: /apideployments/kubernetes/sms-gateway-server/templates/deployment.yaml (1)
17-20: Add config checksum to trigger rollout on ConfigMap changes.Ensures Pods roll on unsecure config updates.
- {{- with .Values.podAnnotations }} - annotations: - {{- toYaml . | nindent 8 }} - {{- end }} + {{- if or .Values.podAnnotations .Values.config.unsecure.enabled }} + annotations: + {{- if .Values.config.unsecure.enabled }} + checksum/config: {{ include (print $.Template.BasePath "/config.yaml") . | sha256sum }} + {{- end }} + {{- with .Values.podAnnotations }} + {{- toYaml . | nindent 8 }} + {{- end }} + {{- end }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
deployments/kubernetes/sms-gateway-server/Chart.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/README.md(1 hunks)deployments/kubernetes/sms-gateway-server/examples/values.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/templates/config.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/templates/deployment.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/templates/hpa.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/templates/ingress.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/templates/service.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/values.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
deployments/kubernetes/sms-gateway-server/Chart.yaml
[error] 16-16: no new line character at the end of file
(new-line-at-end-of-file)
deployments/kubernetes/sms-gateway-server/examples/values.yaml
[error] 31-31: trailing spaces
(trailing-spaces)
[error] 73-73: no new line character at the end of file
(new-line-at-end-of-file)
deployments/kubernetes/sms-gateway-server/templates/config.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
deployments/kubernetes/sms-gateway-server/templates/deployment.yaml
[warning] 7-7: wrong indentation: expected 2 but found 4
(indentation)
[warning] 10-10: wrong indentation: expected 0 but found 2
(indentation)
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
deployments/kubernetes/sms-gateway-server/templates/hpa.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/kubernetes/sms-gateway-server/templates/ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
deployments/kubernetes/sms-gateway-server/templates/service.yaml
[warning] 7-7: wrong indentation: expected 2 but found 4
(indentation)
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
deployments/kubernetes/sms-gateway-server/values.yaml
[warning] 1-1: too many blank lines (1 > 0)
(empty-lines)
[error] 121-121: trailing spaces
(trailing-spaces)
[warning] 148-148: too many blank lines (1 > 0)
(empty-lines)
🪛 LanguageTool
deployments/kubernetes/sms-gateway-server/README.md
[grammar] ~21-~21: There might be a mistake here.
Context: ...al Secret** (Recommended for production) 2. Unsecure ConfigMap (Development/testin...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ...secure ConfigMap** (Development/testing) 3. Environment Variables (Override specif...
(QB_NEW_EN)
[grammar] ~31-~31: There might be a mistake here.
Context: ...age tag | "" (uses Chart.AppVersion) | | image.pullPolicy | Image pull policy...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ...| Image pull policy |IfNotPresent| |service.type` | Kubernetes service ty...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ... Kubernetes service type | ClusterIP | | service.port | Service port | 8080...
(QB_NEW_EN)
[grammar] ~38-~38: There might be a mistake here.
Context: ...n | Parameter | Description | Default | |-----------|-------------|---------| | ...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ... | |-----------|-------------|---------| | podSecurityContext | Pod security co...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...Context| Pod security context |{}| |securityContext` | Container security...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...t| Container security context |{}| |imagePullSecrets` | Image pull secret...
(QB_NEW_EN)
[grammar] ~46-~46: There might be a mistake here.
Context: ...n | Parameter | Description | Default | |-----------|-------------|---------| | ...
(QB_NEW_EN)
[grammar] ~47-~47: There might be a mistake here.
Context: ... | |-----------|-------------|---------| | autoscaling.enabled | Enable horizon...
(QB_NEW_EN)
[grammar] ~48-~48: There might be a mistake here.
Context: ...le horizontal pod autoscaler | false | | autoscaling.minReplicas | Minimum nu...
(QB_NEW_EN)
[grammar] ~49-~49: There might be a mistake here.
Context: ...as| Minimum number of replicas |1| |autoscaling.maxReplicas` | Maximum nu...
(QB_NEW_EN)
[grammar] ~50-~50: There might be a mistake here.
Context: ...| Maximum number of replicas |100| |autoscaling.targetCPUUtilizationPerce...
(QB_NEW_EN)
[grammar] ~51-~51: There might be a mistake here.
Context: ...ntage| Target CPU utilization |80| |autoscaling.targetMemoryUtilizationPe...
(QB_NEW_EN)
[grammar] ~56-~56: There might be a mistake here.
Context: ...n | Parameter | Description | Default | |-----------|-------------|---------| | ...
(QB_NEW_EN)
[grammar] ~57-~57: There might be a mistake here.
Context: ... | |-----------|-------------|---------| | ingress.enabled | Enable ingress | `...
(QB_NEW_EN)
[grammar] ~58-~58: There might be a mistake here.
Context: ...ss.enabled| Enable ingress |false| |ingress.className` | Ingress class na...
(QB_NEW_EN)
[grammar] ~59-~59: There might be a mistake here.
Context: ...className| Ingress class name |""| |ingress.annotations` | Ingress annota...
(QB_NEW_EN)
[grammar] ~60-~60: There might be a mistake here.
Context: ...otations| Ingress annotations |{}| |ingress.hosts[0].host| Hostname |...
(QB_NEW_EN)
[grammar] ~61-~61: There might be a mistake here.
Context: ...st| Hostname |chart-example.local| |ingress.hosts[0].paths[0].path` | Pat...
(QB_NEW_EN)
[grammar] ~62-~62: There might be a mistake here.
Context: ...s.hosts[0].paths[0].path| Path |/| |ingress.hosts[0].paths[0].pathType` |...
(QB_NEW_EN)
[grammar] ~63-~63: There might be a mistake here.
Context: ...| Path type | ImplementationSpecific | | ingress.tls | TLS configuration | `[...
(QB_NEW_EN)
[grammar] ~68-~68: There might be a mistake here.
Context: ...n | Parameter | Description | Default | |-----------|-------------|---------| | ...
(QB_NEW_EN)
[grammar] ~69-~69: There might be a mistake here.
Context: ... | |-----------|-------------|---------| | resources.limits.cpu | CPU limit | `...
(QB_NEW_EN)
[grammar] ~70-~70: There might be a mistake here.
Context: ...urces.limits.cpu| CPU limit |null| |resources.limits.memory` | Memory lim...
(QB_NEW_EN)
[grammar] ~71-~71: There might be a mistake here.
Context: ...limits.memory| Memory limit |null| |resources.requests.cpu` | CPU request...
(QB_NEW_EN)
[grammar] ~72-~72: There might be a mistake here.
Context: ...s.requests.cpu| CPU request |null| |resources.requests.memory` | Memory r...
(QB_NEW_EN)
[grammar] ~77-~77: There might be a mistake here.
Context: ...n | Parameter | Description | Default | |-----------|-------------|---------| | ...
(QB_NEW_EN)
[grammar] ~78-~78: There might be a mistake here.
Context: ... | |-----------|-------------|---------| | nodeSelector | Node selector | {} ...
(QB_NEW_EN)
[grammar] ~79-~79: There might be a mistake here.
Context: ... nodeSelector | Node selector | {} | | tolerations | Tolerations | [] | |...
(QB_NEW_EN)
[grammar] ~80-~80: There might be a mistake here.
Context: ...| | tolerations | Tolerations | [] | | affinity | Affinity rules | {} | ...
(QB_NEW_EN)
[grammar] ~158-~158: There might be a mistake here.
Context: ...nment Variable | Description | Default | |-----------|---------------------|-----...
(QB_NEW_EN)
[grammar] ~159-~159: There might be a mistake here.
Context: ...---------------|-------------|---------| | config.unsecure.configYml (gateway.m...
(QB_NEW_EN)
[grammar] ~160-~160: There might be a mistake here.
Context: ...) or private (protected) | private | | config.unsecure.configYml (gateway.p...
(QB_NEW_EN)
[grammar] ~165-~165: There might be a mistake here.
Context: ...nment Variable | Description | Default | |-----------|---------------------|-----...
(QB_NEW_EN)
[grammar] ~166-~166: There might be a mistake here.
Context: ...---------------|-------------|---------| | config.unsecure.configYml (http.list...
(QB_NEW_EN)
[grammar] ~167-~167: There might be a mistake here.
Context: ...server listen address | 0.0.0.0:8080 | | config.unsecure.configYml (http.prox...
(QB_NEW_EN)
[grammar] ~186-~186: There might be a mistake here.
Context: ...nment Variable | Description | Default | |-----------|---------------------|-----...
(QB_NEW_EN)
[grammar] ~187-~187: There might be a mistake here.
Context: ...---------------|-------------|---------| | config.unsecure.configYml (fcm.crede...
(QB_NEW_EN)
[grammar] ~188-~188: There might be a mistake here.
Context: ...s JSON (for public mode only) | "{}" | | config.unsecure.configYml (fcm.timeo...
(QB_NEW_EN)
[grammar] ~189-~189: There might be a mistake here.
Context: ...| Push notification send timeout | 1 | | config.unsecure.configYml (fcm.debou...
(QB_NEW_EN)
[grammar] ~194-~194: There might be a mistake here.
Context: ...nment Variable | Description | Default | |-----------|---------------------|-----...
(QB_NEW_EN)
[grammar] ~195-~195: There might be a mistake here.
Context: ...---------------|-------------|---------| | config.unsecure.configYml (tasks.has...
(QB_NEW_EN)
[grammar] ~200-~200: There might be a mistake here.
Context: ...nment Variable | Description | Default | |-----------|---------------------|-----...
(QB_NEW_EN)
[grammar] ~201-~201: There might be a mistake here.
Context: ...---------------|-------------|---------| | N/A | HTTP__API__HOST | External API...
(QB_NEW_EN)
[grammar] ~202-~202: There might be a mistake here.
Context: ...nal API host URL for the gateway | N/A | | N/A | HTTP__OPENAPI__ENABLED | Enabl...
(QB_NEW_EN)
[grammar] ~203-~203: There might be a mistake here.
Context: ...enAPI documentation endpoint | false | | N/A | HTTP__API__PATH | API base pat...
(QB_NEW_EN)
[grammar] ~208-~208: There might be a mistake here.
Context: ...n | Parameter | Description | Default | |-----------|-------------|---------| | ...
(QB_NEW_EN)
[grammar] ~209-~209: There might be a mistake here.
Context: ... | |-----------|-------------|---------| | config.debug | Enable debug mode | `...
(QB_NEW_EN)
[grammar] ~210-~210: There might be a mistake here.
Context: ...g.debug| Enable debug mode |false| |extraEnv` | Additional environment va...
(QB_NEW_EN)
🔇 Additional comments (8)
deployments/kubernetes/sms-gateway-server/Chart.yaml (1)
7-9: Confirmed CI workflows correctly automate Chart bumps and releases. The bump workflow triggers on tag pushes and updatesdeployments/kubernetes/sms-gateway-server/Chart.yaml, and the build workflow runschart-releaser-actionagainstdeployments/kubernetesas configured.deployments/kubernetes/sms-gateway-server/README.md (1)
346-349: Fix relative link to config example.Path from this README likely needs one more “../”.
-- **[configs/config.example.yml](../../configs/config.example.yml)** - Complete application configuration reference +- **[configs/config.example.yml](../../../configs/config.example.yml)** - Complete application configuration referencePlease verify the actual location in the repo and adjust accordingly.
deployments/kubernetes/sms-gateway-server/templates/service.yaml (1)
10-16: No action needed: the Deployment template already defines a containerPort named “http” (at line 54), so Service targetPort: http correctly matches.deployments/kubernetes/sms-gateway-server/templates/ingress.yaml (1)
1-1: Ignore templating false positives from plain YAML linters.The leading
{{- if ... -}}confuses yamllint; validate with Helm (helm template/lint) or kubeconform instead.deployments/kubernetes/sms-gateway-server/templates/hpa.yaml (2)
16-42: Metrics guard and defaults look correct.Conditional metrics with a sane CPU default keeps the HPA valid across configs.
14-15: Ensure CPU requests are set when HPA is enabled.Docs/values should make
.resources.requests.cpumandatory for CPU utilization to work. Verify defaults cover this.deployments/kubernetes/sms-gateway-server/examples/values.yaml (1)
69-73: Verify multi-replica readiness before enabling autoscaling.If the app isn’t fully stateless/idempotent, set
autoscaling.enabled: falsein examples to steer users.deployments/kubernetes/sms-gateway-server/templates/deployment.yaml (1)
41-49: Confirm default resources when HPA is enabled.CPU-based HPA requires
resources.requests.cpu. Ensure chart defaults or docs make this explicit.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deployments/kubernetes/sms-gateway-server/README.md (1)
346-349: Fix broken relative link to config.example.yml
In deployments/kubernetes/sms-gateway-server/README.md, update the link on line 348:- **[configs/config.example.yml](../../configs/config.example.yml)** + **[configs/config.example.yml](../../../configs/config.example.yml)**
♻️ Duplicate comments (1)
deployments/kubernetes/sms-gateway-server/README.md (1)
8-14: Helm repo alias and chart reference are now consistent.
🧹 Nitpick comments (6)
deployments/kubernetes/sms-gateway-server/README.md (6)
7-15: Add 'helm repo update' and optional version pin to avoid stale indexes.Prevents "chart not found" after adding repo; version pin helps reproducibility.
```bash helm repo add android-sms-gateway https://android-sms-gateway.github.io/server +helm repo update helm upgrade --install sms-gateway-server \ --namespace sms-gateway-system \ --create-namespace \ - -f your-values.yaml \ + -f your-values.yaml \ + # optionally pin a chart version for reproducible installs: + # --version X.Y.Z \ android-sms-gateway/sms-gateway-server--- `21-24`: **Use “Insecure” instead of “Unsecure” and call out mutual exclusion.** “Unsecure” is nonstandard; also surface that only one config method can be enabled at once (the chart validates this). ```diff -1. **External Secret** (Recommended for production) -2. **Unsecure ConfigMap** (Development/testing) +1. **External Secret** (Recommended for production) +2. **Insecure ConfigMap** (Development/testing) 3. **Environment Variables** (Override specific values) + +Note: Enable exactly one of `config.secret.enabled` or `config.unsecure.enabled`; enabling both is invalid.
100-101: Rename the section headers to match the term “Insecure ConfigMap”.-### Method 2: Unsecure ConfigMap +### Method 2: Insecure ConfigMap-### Method 3: Environment Variables +### Method 3: Environment VariablesAlso applies to: 135-136
111-114: Quote the YAML value containing a colon to avoid parser edge cases.Safer for linters and some YAML parsers.
- http: - listen: 0.0.0.0:8080 + http: + listen: "0.0.0.0:8080"Also applies to: 167-169
251-267: Add a note about production DB best practices.Helpful guardrail for new users deploying with “insecure” config.
### Basic Deployment with External Database @@ database: sms_gateway + +> For production: run your database as a managed/external service with backups +> and durable storage. Avoid in-cluster ephemeral databases unless explicitly +> configured with PersistentVolumeClaims and a proper HA/backup strategy.
319-323: Finish the sentence and strengthen guidance.-**Always use external secrets in production** for sensitive configuration values +**Always use external secrets in production** for sensitive configuration values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
deployments/kubernetes/sms-gateway-server/README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
deployments/kubernetes/sms-gateway-server/README.md
[grammar] ~21-~21: There might be a mistake here.
Context: ...al Secret** (Recommended for production) 2. Unsecure ConfigMap (Development/testin...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ...secure ConfigMap** (Development/testing) 3. Environment Variables (Override specif...
(QB_NEW_EN)
[grammar] ~31-~31: There might be a mistake here.
Context: ...age tag | "" (uses Chart.AppVersion) | | image.pullPolicy | Image pull policy...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ...| Image pull policy |IfNotPresent| |service.type` | Kubernetes service ty...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ... Kubernetes service type | ClusterIP | | service.port | Service port | 8080...
(QB_NEW_EN)
[grammar] ~38-~38: There might be a mistake here.
Context: ...n | Parameter | Description | Default | |-----------|-------------|---------| | ...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ... | |-----------|-------------|---------| | podSecurityContext | Pod security co...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...Context| Pod security context |{}| |securityContext` | Container security...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...t| Container security context |{}| |imagePullSecrets` | Image pull secret...
(QB_NEW_EN)
[grammar] ~46-~46: There might be a mistake here.
Context: ...n | Parameter | Description | Default | |-----------|-------------|---------| | ...
(QB_NEW_EN)
[grammar] ~47-~47: There might be a mistake here.
Context: ... | |-----------|-------------|---------| | autoscaling.enabled | Enable horizon...
(QB_NEW_EN)
[grammar] ~48-~48: There might be a mistake here.
Context: ...le horizontal pod autoscaler | false | | autoscaling.minReplicas | Minimum nu...
(QB_NEW_EN)
[grammar] ~49-~49: There might be a mistake here.
Context: ...as| Minimum number of replicas |1| |autoscaling.maxReplicas` | Maximum nu...
(QB_NEW_EN)
[grammar] ~50-~50: There might be a mistake here.
Context: ...| Maximum number of replicas |100| |autoscaling.targetCPUUtilizationPerce...
(QB_NEW_EN)
[grammar] ~51-~51: There might be a mistake here.
Context: ...ntage| Target CPU utilization |80| |autoscaling.targetMemoryUtilizationPe...
(QB_NEW_EN)
[grammar] ~56-~56: There might be a mistake here.
Context: ...n | Parameter | Description | Default | |-----------|-------------|---------| | ...
(QB_NEW_EN)
[grammar] ~57-~57: There might be a mistake here.
Context: ... | |-----------|-------------|---------| | ingress.enabled | Enable ingress | `...
(QB_NEW_EN)
[grammar] ~58-~58: There might be a mistake here.
Context: ...ss.enabled| Enable ingress |false| |ingress.className` | Ingress class na...
(QB_NEW_EN)
[grammar] ~59-~59: There might be a mistake here.
Context: ...className| Ingress class name |""| |ingress.annotations` | Ingress annota...
(QB_NEW_EN)
[grammar] ~60-~60: There might be a mistake here.
Context: ...otations| Ingress annotations |{}| |ingress.hosts[0].host| Hostname |...
(QB_NEW_EN)
[grammar] ~61-~61: There might be a mistake here.
Context: ...st| Hostname |chart-example.local| |ingress.hosts[0].paths[0].path` | Pat...
(QB_NEW_EN)
[grammar] ~62-~62: There might be a mistake here.
Context: ...s.hosts[0].paths[0].path| Path |/| |ingress.hosts[0].paths[0].pathType` |...
(QB_NEW_EN)
[grammar] ~63-~63: There might be a mistake here.
Context: ...| Path type | ImplementationSpecific | | ingress.tls | TLS configuration | `[...
(QB_NEW_EN)
[grammar] ~68-~68: There might be a mistake here.
Context: ...n | Parameter | Description | Default | |-----------|-------------|---------| | ...
(QB_NEW_EN)
[grammar] ~69-~69: There might be a mistake here.
Context: ... | |-----------|-------------|---------| | resources.limits.cpu | CPU limit | `...
(QB_NEW_EN)
[grammar] ~70-~70: There might be a mistake here.
Context: ...urces.limits.cpu| CPU limit |null| |resources.limits.memory` | Memory lim...
(QB_NEW_EN)
[grammar] ~71-~71: There might be a mistake here.
Context: ...limits.memory| Memory limit |null| |resources.requests.cpu` | CPU request...
(QB_NEW_EN)
[grammar] ~72-~72: There might be a mistake here.
Context: ...s.requests.cpu| CPU request |null| |resources.requests.memory` | Memory r...
(QB_NEW_EN)
[grammar] ~77-~77: There might be a mistake here.
Context: ...n | Parameter | Description | Default | |-----------|-------------|---------| | ...
(QB_NEW_EN)
[grammar] ~78-~78: There might be a mistake here.
Context: ... | |-----------|-------------|---------| | nodeSelector | Node selector | {} ...
(QB_NEW_EN)
[grammar] ~79-~79: There might be a mistake here.
Context: ... nodeSelector | Node selector | {} | | tolerations | Tolerations | [] | |...
(QB_NEW_EN)
[grammar] ~80-~80: There might be a mistake here.
Context: ...| | tolerations | Tolerations | [] | | affinity | Affinity rules | {} | ...
(QB_NEW_EN)
[grammar] ~158-~158: There might be a mistake here.
Context: ...nment Variable | Description | Default | |-----------|---------------------|-----...
(QB_NEW_EN)
[grammar] ~159-~159: There might be a mistake here.
Context: ...---------------|-------------|---------| | config.unsecure.configYml (gateway.m...
(QB_NEW_EN)
[grammar] ~160-~160: There might be a mistake here.
Context: ...) or private (protected) | private | | config.unsecure.configYml (gateway.p...
(QB_NEW_EN)
[grammar] ~165-~165: There might be a mistake here.
Context: ...nment Variable | Description | Default | |-----------|---------------------|-----...
(QB_NEW_EN)
[grammar] ~166-~166: There might be a mistake here.
Context: ...---------------|-------------|---------| | config.unsecure.configYml (http.list...
(QB_NEW_EN)
[grammar] ~167-~167: There might be a mistake here.
Context: ...server listen address | 0.0.0.0:8080 | | config.unsecure.configYml (http.prox...
(QB_NEW_EN)
[grammar] ~186-~186: There might be a mistake here.
Context: ...nment Variable | Description | Default | |-----------|---------------------|-----...
(QB_NEW_EN)
[grammar] ~187-~187: There might be a mistake here.
Context: ...---------------|-------------|---------| | config.unsecure.configYml (fcm.crede...
(QB_NEW_EN)
[grammar] ~188-~188: There might be a mistake here.
Context: ...s JSON (for public mode only) | "{}" | | config.unsecure.configYml (fcm.timeo...
(QB_NEW_EN)
[grammar] ~189-~189: There might be a mistake here.
Context: ...| Push notification send timeout | 1 | | config.unsecure.configYml (fcm.debou...
(QB_NEW_EN)
[grammar] ~194-~194: There might be a mistake here.
Context: ...nment Variable | Description | Default | |-----------|---------------------|-----...
(QB_NEW_EN)
[grammar] ~195-~195: There might be a mistake here.
Context: ...---------------|-------------|---------| | config.unsecure.configYml (tasks.has...
(QB_NEW_EN)
[grammar] ~200-~200: There might be a mistake here.
Context: ...nment Variable | Description | Default | |-----------|---------------------|-----...
(QB_NEW_EN)
[grammar] ~201-~201: There might be a mistake here.
Context: ...---------------|-------------|---------| | N/A | HTTP__API__HOST | External API...
(QB_NEW_EN)
[grammar] ~202-~202: There might be a mistake here.
Context: ...nal API host URL for the gateway | N/A | | N/A | HTTP__OPENAPI__ENABLED | Enabl...
(QB_NEW_EN)
[grammar] ~203-~203: There might be a mistake here.
Context: ...enAPI documentation endpoint | false | | N/A | HTTP__API__PATH | API base pat...
(QB_NEW_EN)
[grammar] ~208-~208: There might be a mistake here.
Context: ...n | Parameter | Description | Default | |-----------|-------------|---------| | ...
(QB_NEW_EN)
[grammar] ~209-~209: There might be a mistake here.
Context: ... | |-----------|-------------|---------| | config.debug | Enable debug mode | `...
(QB_NEW_EN)
[grammar] ~210-~210: There might be a mistake here.
Context: ...g.debug| Enable debug mode |false| |extraEnv` | Additional environment va...
(QB_NEW_EN)
🔇 Additional comments (2)
deployments/kubernetes/sms-gateway-server/README.md (2)
30-31: Image repository default aligns with values.yaml. LGTM.
217-231: Probes correctly reference the named container port “http”; no update needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
deployments/kubernetes/sms-gateway-server/README.md (4)
34-34: Use Helm’s.Chart.AppVersionnotation.Change “Chart.AppVersion” to “.Chart.AppVersion” for accuracy.
-| `image.tag` | Container image tag | `""` (uses Chart.AppVersion) | +| `image.tag` | Container image tag | `""` (uses .Chart.AppVersion) |
90-101: Add a quick-start command for creating the external secret.A one-liner improves DX.
Your secret should contain a `config.yml` key with the full configuration. +Example: + +```bash +kubectl create secret generic my-sms-gateway-config \ + --from-file=config.yml=./configs/config.example.yml \ + -n sms-gateway-system +```
5-18: Add uninstall and dry-run snippets to Installation.Helps users validate and roll back easily.
# --version X.Y.Z \ android-sms-gateway/sms-gateway-server+### Validate and uninstall
+
+bash +# Render without applying +helm template sms-gateway-server android-sms-gateway/sms-gateway-server -n sms-gateway-system + +# Uninstall +helm uninstall sms-gateway-server -n sms-gateway-system +--- `254-275`: **Document optional in-cluster MariaDB (dev-only) if/when added.** If you merge the optional DB template, gate it by a value and warn about persistence. ```diff ### Basic Deployment with External Database @@ > and durable storage. Avoid in-cluster ephemeral databases unless explicitly > configured with PersistentVolumeClaims and a proper HA/backup strategy. +### (Optional) Dev-only internal MariaDB + +> Only if the chart includes an internal DB template guarded by a value. +> Not recommended for production. + +```yaml +database: + deployInternal: true + internal: + image: mariadb:11 + storage: + size: 5Gi + storageClass: "" # set to your StorageClass + credentials: + rootPassword: "change-me" + database: "sms" + user: "sms_user" + password: "change-me-too" +``` + +Ensure a PVC is provisioned and backups are configured before storing data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
deployments/kubernetes/sms-gateway-server/README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
deployments/kubernetes/sms-gateway-server/README.md
[grammar] ~24-~24: There might be a mistake here.
Context: ...al Secret** (Recommended for production) 2. Unsecure ConfigMap (Development/testin...
(QB_NEW_EN)
[grammar] ~25-~25: There might be a mistake here.
Context: ...secure ConfigMap** (Development/testing) 3. Environment Variables (Override specif...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...age tag | "" (uses Chart.AppVersion) | | image.pullPolicy | Image pull policy...
(QB_NEW_EN)
[grammar] ~35-~35: There might be a mistake here.
Context: ...| Image pull policy |IfNotPresent| |service.type` | Kubernetes service ty...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ... Kubernetes service type | ClusterIP | | service.port | Service port | 8080...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...n | Parameter | Description | Default | |-----------|-------------|---------| | ...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ... | |-----------|-------------|---------| | podSecurityContext | Pod security co...
(QB_NEW_EN)
[grammar] ~43-~43: There might be a mistake here.
Context: ...Context| Pod security context |{}| |securityContext` | Container security...
(QB_NEW_EN)
[grammar] ~44-~44: There might be a mistake here.
Context: ...t| Container security context |{}| |imagePullSecrets` | Image pull secret...
(QB_NEW_EN)
[grammar] ~49-~49: There might be a mistake here.
Context: ...n | Parameter | Description | Default | |-----------|-------------|---------| | ...
(QB_NEW_EN)
[grammar] ~50-~50: There might be a mistake here.
Context: ... | |-----------|-------------|---------| | autoscaling.enabled | Enable horizon...
(QB_NEW_EN)
[grammar] ~51-~51: There might be a mistake here.
Context: ...le horizontal pod autoscaler | false | | autoscaling.minReplicas | Minimum nu...
(QB_NEW_EN)
[grammar] ~52-~52: There might be a mistake here.
Context: ...as| Minimum number of replicas |1| |autoscaling.maxReplicas` | Maximum nu...
(QB_NEW_EN)
[grammar] ~53-~53: There might be a mistake here.
Context: ...| Maximum number of replicas |100| |autoscaling.targetCPUUtilizationPerce...
(QB_NEW_EN)
[grammar] ~54-~54: There might be a mistake here.
Context: ...ntage| Target CPU utilization |80| |autoscaling.targetMemoryUtilizationPe...
(QB_NEW_EN)
[grammar] ~59-~59: There might be a mistake here.
Context: ...n | Parameter | Description | Default | |-----------|-------------|---------| | ...
(QB_NEW_EN)
[grammar] ~60-~60: There might be a mistake here.
Context: ... | |-----------|-------------|---------| | ingress.enabled | Enable ingress | `...
(QB_NEW_EN)
[grammar] ~61-~61: There might be a mistake here.
Context: ...ss.enabled| Enable ingress |false| |ingress.className` | Ingress class na...
(QB_NEW_EN)
[grammar] ~62-~62: There might be a mistake here.
Context: ...className| Ingress class name |""| |ingress.annotations` | Ingress annota...
(QB_NEW_EN)
[grammar] ~63-~63: There might be a mistake here.
Context: ...otations| Ingress annotations |{}| |ingress.hosts[0].host| Hostname |...
(QB_NEW_EN)
[grammar] ~64-~64: There might be a mistake here.
Context: ...st| Hostname |chart-example.local| |ingress.hosts[0].paths[0].path` | Pat...
(QB_NEW_EN)
[grammar] ~65-~65: There might be a mistake here.
Context: ...s.hosts[0].paths[0].path| Path |/| |ingress.hosts[0].paths[0].pathType` |...
(QB_NEW_EN)
[grammar] ~66-~66: There might be a mistake here.
Context: ...| Path type | ImplementationSpecific | | ingress.tls | TLS configuration | `[...
(QB_NEW_EN)
[grammar] ~71-~71: There might be a mistake here.
Context: ...n | Parameter | Description | Default | |-----------|-------------|---------| | ...
(QB_NEW_EN)
[grammar] ~72-~72: There might be a mistake here.
Context: ... | |-----------|-------------|---------| | resources.limits.cpu | CPU limit | `...
(QB_NEW_EN)
[grammar] ~73-~73: There might be a mistake here.
Context: ...urces.limits.cpu| CPU limit |null| |resources.limits.memory` | Memory lim...
(QB_NEW_EN)
[grammar] ~74-~74: There might be a mistake here.
Context: ...limits.memory| Memory limit |null| |resources.requests.cpu` | CPU request...
(QB_NEW_EN)
[grammar] ~75-~75: There might be a mistake here.
Context: ...s.requests.cpu| CPU request |null| |resources.requests.memory` | Memory r...
(QB_NEW_EN)
[grammar] ~80-~80: There might be a mistake here.
Context: ...n | Parameter | Description | Default | |-----------|-------------|---------| | ...
(QB_NEW_EN)
[grammar] ~81-~81: There might be a mistake here.
Context: ... | |-----------|-------------|---------| | nodeSelector | Node selector | {} ...
(QB_NEW_EN)
[grammar] ~82-~82: There might be a mistake here.
Context: ... nodeSelector | Node selector | {} | | tolerations | Tolerations | [] | |...
(QB_NEW_EN)
[grammar] ~83-~83: There might be a mistake here.
Context: ...| | tolerations | Tolerations | [] | | affinity | Affinity rules | {} | ...
(QB_NEW_EN)
[grammar] ~161-~161: There might be a mistake here.
Context: ...nment Variable | Description | Default | |-----------|---------------------|-----...
(QB_NEW_EN)
[grammar] ~162-~162: There might be a mistake here.
Context: ...---------------|-------------|---------| | config.unsecure.configYml (gateway.m...
(QB_NEW_EN)
[grammar] ~163-~163: There might be a mistake here.
Context: ...) or private (protected) | private | | config.unsecure.configYml (gateway.p...
(QB_NEW_EN)
[grammar] ~168-~168: There might be a mistake here.
Context: ...nment Variable | Description | Default | |-----------|---------------------|-----...
(QB_NEW_EN)
[grammar] ~169-~169: There might be a mistake here.
Context: ...---------------|-------------|---------| | config.unsecure.configYml (http.list...
(QB_NEW_EN)
[grammar] ~170-~170: There might be a mistake here.
Context: ...server listen address | 0.0.0.0:8080 | | config.unsecure.configYml (http.prox...
(QB_NEW_EN)
[grammar] ~189-~189: There might be a mistake here.
Context: ...nment Variable | Description | Default | |-----------|---------------------|-----...
(QB_NEW_EN)
[grammar] ~190-~190: There might be a mistake here.
Context: ...---------------|-------------|---------| | config.unsecure.configYml (fcm.crede...
(QB_NEW_EN)
[grammar] ~191-~191: There might be a mistake here.
Context: ...s JSON (for public mode only) | "{}" | | config.unsecure.configYml (fcm.timeo...
(QB_NEW_EN)
[grammar] ~192-~192: There might be a mistake here.
Context: ...| Push notification send timeout | 1 | | config.unsecure.configYml (fcm.debou...
(QB_NEW_EN)
[grammar] ~197-~197: There might be a mistake here.
Context: ...nment Variable | Description | Default | |-----------|---------------------|-----...
(QB_NEW_EN)
[grammar] ~198-~198: There might be a mistake here.
Context: ...---------------|-------------|---------| | config.unsecure.configYml (tasks.has...
(QB_NEW_EN)
[grammar] ~203-~203: There might be a mistake here.
Context: ...nment Variable | Description | Default | |-----------|---------------------|-----...
(QB_NEW_EN)
[grammar] ~204-~204: There might be a mistake here.
Context: ...---------------|-------------|---------| | N/A | HTTP__API__HOST | External API...
(QB_NEW_EN)
[grammar] ~205-~205: There might be a mistake here.
Context: ...nal API host URL for the gateway | N/A | | N/A | HTTP__OPENAPI__ENABLED | Enabl...
(QB_NEW_EN)
[grammar] ~206-~206: There might be a mistake here.
Context: ...enAPI documentation endpoint | false | | N/A | HTTP__API__PATH | API base pat...
(QB_NEW_EN)
[grammar] ~211-~211: There might be a mistake here.
Context: ...n | Parameter | Description | Default | |-----------|-------------|---------| | ...
(QB_NEW_EN)
[grammar] ~212-~212: There might be a mistake here.
Context: ... | |-----------|-------------|---------| | config.debug | Enable debug mode | `...
(QB_NEW_EN)
[grammar] ~213-~213: There might be a mistake here.
Context: ...g.debug| Enable debug mode |false| |extraEnv` | Additional environment va...
(QB_NEW_EN)
🔇 Additional comments (3)
deployments/kubernetes/sms-gateway-server/README.md (3)
7-18: Note on repo endpoint stability.If publishing switches from GitHub Pages to S3, remember to update the repo URL and alias here to prevent install failures.
-helm repo add android-sms-gateway https://android-sms-gateway.github.io/server +helm repo add android-sms-gateway https://android-sms-gateway.github.io/server +# If publishing moves to S3 later, update the URL above accordingly.
353-356: All referenced README file paths verified. Bothexamples/values.yamlandconfigs/config.example.ymlexist, so the links are valid.
221-234: Named porthttpis defined in both Deployment (containerPort namedhttp) and Service (targetPort:http) templates, so liveness/readiness probes will work as expected.
Yes, I think
Yes, I could.
|
There was a problem hiding this comment.
Another thing I don't like is the commit that is not created and reviewed by a human. I understand that this is a fairly obvious change and won't break anything, but I would like to have full control over everything that happens in the master branch.
Another problem is that the commit will be sent after the version tag and will not be directly related to it.
Let's use a simpler solution:
- Update the chart version manually. I don't think it will be updated often.
- Update
appVersiondynamically when the chart is released, by getting the latest GitHub release version or the latestv*tag.
There was a problem hiding this comment.
- Update
appVersiondynamically when the chart is released, by getting the latest GitHub release version or the latestv*tag.
Okay good, in any case:
- whenever someone wants to use the container version anyways, they can still use
- image:
tag: "vx.x.x"
Let me know and i make the changes
|
|
||
| ## Configuration Reference | ||
|
|
||
| Based on the example configuration file, here are all available configuration options: |
There was a problem hiding this comment.
Do we really need to list all the available options? Isn't the link to the example config in the repository enough?
There was a problem hiding this comment.
Whenever someone want to create a values.yaml, they sometimes look at the docs, it is helpful. Even if it is not updated it is at least there.
|
|
||
| | Parameter | Environment Variable | Description | Default | | ||
| |-----------|---------------------|-------------|---------| | ||
| | N/A | `HTTP__API__HOST` | External API host URL for the gateway | N/A | |
There was a problem hiding this comment.
These options are also available via yml.
There was a problem hiding this comment.
Correct. When it will be merged, there will be no difference, because it will be pointing to the config example
|
And please don't forget to review the CodeRabbit AI nitpick comments. Sometimes they contain useful ideas. |
Yes, i try to use the nickpick comments, but sometimes it doesn't make sense. I consider all the coderabbit as done. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
deployments/kubernetes/sms-gateway-server/examples/values.yaml (1)
5-5: replicaCount set to 1 — good alignment with current single-instance limitationMatches maintainer guidance to avoid multiple replicas.
🧹 Nitpick comments (4)
deployments/kubernetes/sms-gateway-server/examples/values.yaml (4)
7-10: Use IfNotPresent pull policy for tagged releasesPulling on every start is wasteful when using immutable tags. Prefer IfNotPresent in the example.
Apply:
image: repository: ghcr.io/android-sms-gateway/server - pullPolicy: Always + pullPolicy: IfNotPresent
23-34: Example enables secret but doesn’t ensure it exists — flip default and document creationAs-is, installs will fail unless users pre-create the Secret. Make the example safer and self-explanatory.
Apply:
config: debug: false secret: - enabled: true + enabled: false # Define your secret and deploy it with any name below but it should be in the same namespace or use "sms-gateway-server-config" # see configs/config.example.yml secretName: "sms-gateway-config" + # To create the secret in advance (example): + # kubectl create secret generic sms-gateway-config \ + # --from-file=config.yml=./config.yml -n <namespace> + # Enable either config.secret or config.unsecure, not both. # If you do not care, just enable unsecure, it will create # a config for you and it will apply what you configured. # configs/config.example.yml, DO NOT USE the default config below unsecure: enabled: false
31-31: Trim trailing spaceYAMLlint flagged trailing space on this line.
Apply:
- # a config for you and it will apply what you configured. + # a config for you and it will apply what you configured.
73-73: Add a newline at EOFSatisfy linters and POSIX tools expecting a trailing newline.
Apply:
targetCPUUtilizationPercentage: 80 +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
deployments/kubernetes/sms-gateway-server/examples/values.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
deployments/kubernetes/sms-gateway-server/examples/values.yaml
[error] 31-31: trailing spaces
(trailing-spaces)
[error] 73-73: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
deployments/kubernetes/sms-gateway-server/examples/values.yaml (1)
61-67: Neutral host looks good; env keys read cleanlyNice switch to sms.example.com and explicit OpenAPI/paths.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
deployments/kubernetes/sms-gateway-server/Chart.yaml (1)
12-17: Metadata addition matches prior feedback.home, sources, and maintainers are present as requested earlier.
🧹 Nitpick comments (3)
deployments/kubernetes/sms-gateway-server/Chart.yaml (3)
5-5: Add kubeVersion, keywords, and basic annotations (Artifact Hub).This improves discovery and prevents installs on unsupported clusters.
Apply:
type: application + +kubeVersion: ">=1.25.0-0" +keywords: + - sms + - gateway + - android +annotations: + artifacthub.io/links: | + - name: Source + url: https://github.com/android-sms-gateway/server
17-17: Add newline at EOF.YAMLlint flags missing trailing newline.
Apply:
email: [email protected] +
15-17: If you decide to bundle an optional DB, prefer a dependency over in-repo templates.A conditioned dependency keeps templates lean and lets you inherit upstream maintenance.
Apply (and remember to run “helm dependency update” in CI):
- name: SMSGate Team email: [email protected] + +dependencies: + - name: mariadb + repository: oci://registry-1.docker.io/bitnamicharts + version: "^18.0.0" + condition: database.deployInternal
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
deployments/kubernetes/sms-gateway-server/Chart.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
deployments/kubernetes/sms-gateway-server/Chart.yaml
[error] 17-17: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (2)
deployments/kubernetes/sms-gateway-server/Chart.yaml (2)
1-6: Core chart metadata is correct.apiVersion v2, name, description, and type are valid.
7-7: Align Chart.yaml bump with CI workflow
CI’s bump-chart-app-version.yaml updates bothversionandappVersion(lines 40–41); if your release process should only bumpappVersion, remove this manual Chart version bump or adjust the CI workflow triggers accordingly.
I made some changes, so it is irrelevant now. What happens, the version is static. appVersion only changes on release, by default it is latest, so that we have a default value.
I let you deal with that one, Github is convenient for me for quickly test and deploy. Also I think when the s3 is done, and running. Bump it to 1.0.0. So that it discard all those version i made.
Yeah, Statefulset isn't replication, it only retain the pod identity. And you said it right:
But still, each Statefulset with a storage class will mount a different PVC per node. Basically node 1-3, has pvc 1-3, it is not the same, you will have to fiddle around to make replication, and so on, so it could work. The docs you mentionned makes it clear that, Statefulset =/= redundancy and replication. 
But nothing stops us from providing a step by step guide to deploy a cloud native mariadb using helm chart. Example; mention this in the docs: https://github.com/mariadb-operator/mariadb-operator/blob/main/docs/helm.md |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
deployments/kubernetes/sms-gateway-server/Chart.yaml (1)
9-10: “latest” appVersion is fine given CI bump—ensure safe local defaults.Per the learnings, CI replaces appVersion from tags. For local installs (before CI runs), consider defaulting values.image.tag explicitly so you don’t pull a moving latest if someone installs from the working tree.
🧹 Nitpick comments (6)
deployments/kubernetes/sms-gateway-server/Chart.yaml (2)
1-6: Consider declaring supported Kubernetes versions.Add kubeVersion to signal compatibility (e.g., ">=1.26.0-0").
apiVersion: v2 name: sms-gateway-server description: A Helm chart for Kubernetes to deploy sms-gateway-server. type: application +kubeVersion: ">=1.26.0-0"
17-17: Add a trailing newline.Fixes the YAML lint warning.
- email: [email protected] + email: [email protected] +.github/workflows/build-chart.yaml (4)
17-21: Fetch tags explicitly to make git describe reliable.Some runners won’t fetch tags unless asked.
- name: Checkout uses: actions/checkout@v4 with: fetch-depth: 0 + fetch-tags: true
49-55: Optionally skip re-uploading existing releases.Prevents failures when re-running on the same chart version.
- name: Run chart-releaser id: cr uses: helm/[email protected] env: CR_TOKEN: "${{ secrets.GITHUB_TOKEN }}" with: charts_dir: ./deployments/kubernetes + skip_existing: true
13-55: Add lightweight lint/test before release.Consider chart-testing to lint and install the chart in kind to catch template issues early.
jobs: release: runs-on: ubuntu-latest steps: + - name: Set up Helm chart testing + uses: helm/chart-testing-action@v2 + - name: Lint chart + run: ct lint --charts deployments/kubernetes/sms-gateway-server
55-55: Add a trailing newline.Silences the YAML lint warning.
- charts_dir: ./deployments/kubernetes + charts_dir: ./deployments/kubernetes +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/build-chart.yaml(1 hunks)deployments/kubernetes/sms-gateway-server/Chart.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.052Z
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.
📚 Learning: 2025-09-05T08:37:28.052Z
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.052Z
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:
.github/workflows/build-chart.yamldeployments/kubernetes/sms-gateway-server/Chart.yaml
🪛 YAMLlint (1.37.1)
.github/workflows/build-chart.yaml
[error] 55-55: no new line character at the end of file
(new-line-at-end-of-file)
deployments/kubernetes/sms-gateway-server/Chart.yaml
[error] 17-17: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (2)
deployments/kubernetes/sms-gateway-server/Chart.yaml (2)
12-17: Metadata block looks good.home, sources, and maintainers are present and well-formed. Nice.
7-7: Confirm chart versioning policy.version: 0.2.11 implies you’ll bump it on any chart change (independent of appVersion). Just confirming this is your intended workflow.
I apologize, but where in this discussion has anyone mentioned replication or redundancy until now? My response was specifically addressing your concern that "a node failure would reschedule the database on another node, but the data would be lost." A properly configured Let's please not overcomplicate the discussion by introducing requirements that weren't originally mentioned. |
|
Sorry about that, but I think i will keep it simple, it covers whats we need deploying the server, and just need to plug and database to it, and i do not want to add another dependency. You can always extend it. it is not v1, it is still on perpetual changes, but i think i covered a major part of it. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.github/workflows/build-chart.yaml (4)
31-37: Consider a more robust tag selector.
git describe --tags --abbrev=0finds the latest tag reachable from HEAD. If you ever want “latest semantic tag” regardless of reachability (e.g., after rebases), switch to a sorted query.Example:
- TAG=$(git describe --tags --abbrev=0 2>/dev/null || echo "") + TAG=$(git tag --list --sort=-v:refname | head -n1 || echo "")
13-16: Optional: add concurrency to prevent overlapping publishes.Cancel in-flight runs on rapid successive pushes.
jobs: release: runs-on: ubuntu-latest + concurrency: + group: charts-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true
41-49: Future-proof for multiple charts.The path hard-codes
sms-gateway-server. If more charts land underdeployments/kubernetes/, loop over allChart.yamlfiles.Example shell (if you choose to install yq into PATH instead):
- FILE="deployments/kubernetes/sms-gateway-server/Chart.yaml" - echo "Temporarily updating appVersion in $FILE to $APPVERSION for release" - yq -i ".appVersion = \"$APPVERSION\"" "$FILE" + find deployments/kubernetes -name Chart.yaml -print0 | while IFS= read -r -d '' FILE; do + echo "Temporarily updating appVersion in $FILE to $APPVERSION for release" + yq -i ".appVersion = \"$APPVERSION\"" "$FILE" + done
50-56: Upgrade helm/chart-releaser-action to v1.7.0 – latest stable release (no breaking changes since v1.5.0).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/build-chart.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.052Z
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.
📚 Learning: 2025-09-05T08:37:28.052Z
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.052Z
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:
.github/workflows/build-chart.yaml
🔇 Additional comments (5)
.github/workflows/build-chart.yaml (5)
3-9: Trigger scope looks good.Push-to-master with a paths filter for
deployments/kubernetes/**matches the maintainer’s intent and avoids unnecessary runs.
10-12: Permissions are right-sized.
contents: writeis sufficient for chart-releaser to create releases and push to gh-pages.
50-56: Ensure gh-pages (or chosen charts branch) exists before first run.chart-releaser expects the target branch to exist. Confirm
gh-pagesis created (with.nojekyll) or configure a different branch per your publishing choice (e.g., S3 alternative).Would you like a preflight step that creates
gh-pagesif missing?
17-22: Checkout config LGTM.
fetch-depth: 0andfetch-tags: truecorrectly support tag-based versioning.
1-56: Avoid workflow overlap with the separate bump-appVersion CI.Per learnings, another workflow auto-bumps
appVersionfrom git tags. Ensure both don’t conflict or double-publish.Do you want this workflow to do only a temporary bump (no commit) while the other commits the change? If yes, current approach is fine after fixing yq usage.
| - name: Install yq | ||
| uses: mikefarah/yq@v4 | ||
|
|
There was a problem hiding this comment.
Fix: yq action is misused; subsequent yq -i will fail.
uses: mikefarah/yq@v4 does not “install” yq for later steps; it executes yq via its own cmd: input. The current setup likely fails at Line 48 because yq isn’t on PATH. Use the action to run the edit, and export APPVERSION once when deriving it from the tag.
Apply:
@@
- - name: Install yq
- uses: mikefarah/yq@v4
+ # (remove separate install; we'll invoke yq via its action below)
@@
- name: Get last Git tag
id: get_tag
# Forks will not have tags, so it won't change the version
run: |
TAG=$(git describe --tags --abbrev=0 2>/dev/null || echo "")
echo "tag=$TAG" >> $GITHUB_OUTPUT
+ # Derive app version once and export for later steps
+ APPVERSION="${TAG#v}"
+ echo "APPVERSION=$APPVERSION" >> $GITHUB_ENV
@@
- - name: Update appVersion in Chart.yaml for release
- if: steps.get_tag.outputs.tag != ''
- run: |
- FILE="deployments/kubernetes/sms-gateway-server/Chart.yaml"
- TAG="${{ steps.get_tag.outputs.tag }}"
-
- # Strip leading 'v' if present
- APPVERSION="${TAG#v}"
-
- echo "Temporarily updating appVersion in $FILE to $APPVERSION for release"
- yq -i ".appVersion = \"$APPVERSION\"" "$FILE"
+ - name: Update appVersion in Chart.yaml for release
+ if: steps.get_tag.outputs.tag != ''
+ uses: mikefarah/yq@v4
+ env:
+ FILE: deployments/kubernetes/sms-gateway-server/Chart.yaml
+ with:
+ cmd: yq -i '.appVersion = strenv(APPVERSION)' "$FILE"Also applies to: 38-49
🤖 Prompt for AI Agents
.github/workflows/build-chart.yaml lines 28-30 (also applies to 38-49): the
workflow wrongly uses "uses: mikefarah/yq@v4" as if it installs yq to PATH for
later steps, which causes later "yq -i" invocations to fail; replace the
separate install step by invoking the mikefarah/yq action with its "cmd" input
to perform the in-place edits (or install yq via apt/npm if you truly need it on
PATH), and when extracting APPVERSION from the tag compute and export APPVERSION
once (export APPVERSION=<value>) so subsequent steps can reuse it instead of
recalculating; update the affected steps so they either run yq edits via the
action or ensure a real yq binary is installed and APPVERSION is exported as an
environment variable.
One of my primary goals when introducing Private mode was to make deployment as simple as possible for users. That's why all current deployment examples include database deployment. In the Helm chart, the database won't be a hard dependency but rather an optional component for quick starts. Personally, when I explore a new project, I want a simple and quick way to deploy it in a test environment. I want to provide this same convenience to users of my project.
I truly appreciate your contribution, but as I mentioned previously, I hadn't originally planned for Kubernetes deployment of this application. I also don't possess extensive expertise in this area. However, I'm committed to providing a quality product. If you simply provide the Helm chart as-is and then move on, I won't be able to properly maintain it. That's why I'm trying to make the Helm chart as ideal as possible from the start. |
|
Gotcha, i'll make the changes! |
Steps:
This will create a chart release where any kubernetes people can do:
helm repo add android-sms-gateway https://android-sms-gateway.github.io/server helm upgrade --install android-sms-gateway \ --namespace sms-gateway-system \ --create-namespace \ android-sms-gateway/sms-gateway-server # can append their values for further configurationSummary by CodeRabbit