Improved Helm Chart Templates#215
Conversation
Signed-off-by: Sascha Di Bernardo <Sascha.diBernardo@viadee.de>
Signed-off-by: Sascha Di Bernardo <Sascha.diBernardo@viadee.de>
Signed-off-by: Sascha Di Bernardo <Sascha.diBernardo@viadee.de>
Signed-off-by: Sascha Di Bernardo <Sascha.diBernardo@viadee.de>
WalkthroughTemplates were converted from a single aggregated service config to per-service resources: one ConfigMap, volume, mount, port, and test container per entry in .Values.beelzebubServiceConfigs; names and ports are derived from each entry’s name and config.address. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Helm User
participant H as Helm Template Engine
participant K as Kubernetes API
participant D as Deployment/Pod
participant S as Service
rect rgba(227,246,255,0.5)
note over U,H: Template render using .Values.beelzebubServiceConfigs
U->>H: helm install/upgrade with values
H-->>U: Manifests: N ConfigMaps, Deployment, Service, Test Pod
end
rect rgba(240,255,240,0.5)
note over U,K: Apply manifests
U->>K: kubectl apply (Helm)
K-->>U: Creates N ConfigMaps (beelzebub-services-cm-<name>)
K-->>U: Updates Deployment (adds N volumes/mounts, containerPorts)
K-->>U: Updates Service (adds N named ports)
end
rect rgba(255,249,227,0.5)
note over D,S: Runtime wiring
K-->>D: Pod mounts per-service ConfigMaps (subPath = entry name)
K-->>S: Service exposes named ports per entry
D-->>S: Containers listen on ports derived from each config.address
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ 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 (
|
|
I feel like to original Helm Chart was not that easy to work with out of the box and thus decided to propose changes as seen in the PR. Feel free to discuss and I'm open to any kind of feedback. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 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 (3)
beelzebub-chart/templates/configmap_services.yaml(1 hunks)beelzebub-chart/templates/deployment.yaml(2 hunks)beelzebub-chart/templates/ingress.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
beelzebub-chart/templates/configmap_services.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (1)
beelzebub-chart/templates/deployment.yaml (1)
40-45: Sanitize volume names & guard subPath
- Derive and use
$safeNameforname: config-services-volume-…exactly as you do for the ConfigMap to keep them in sync.- Ensure
$value.namecannot contain/(invalid forsubPath) or sanitize it upstream.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
beelzebub-chart/templates/configmap_services.yaml (1)
1-2: Compute and reuse a DNS-1123-safe name per item; use it for resource names.$value.name can contain dots/uppercase and break K8s naming. Compute $safeName once and reuse.
Apply:
-{{- range $value := .Values.beelzebubServiceConfigs }} +{{- range $value := .Values.beelzebubServiceConfigs }} +{{- $trimmed := trimSuffix ".yaml" $value.name -}} +{{- $safeName := (regexReplaceAll "[^a-z0-9-]" (lower $trimmed) "-" ) | trimAll "-" | trunc 63 -}} --- apiVersion: v1 kind: ConfigMap metadata: - name: beelzebub-services-cm-{{ trimSuffix ".yaml" $value.name }} + name: beelzebub-services-cm-{{ $safeName }}Also applies to: 6-6
beelzebub-chart/templates/deployment.yaml (2)
40-46: Use a single sanitized name for volumeMount names; keep user-facing paths intact.Volume names must be DNS-1123 compliant. Compute $safeName once and reuse.
- {{- range $index, $value := .Values.beelzebubServiceConfigs }} - - name: config-services-volume-{{ trimSuffix ".yaml" $value.name }} + {{- range $index, $value := .Values.beelzebubServiceConfigs }} + {{- $trimmed := trimSuffix ".yaml" $value.name -}} + {{- $safeName := (regexReplaceAll "[^a-z0-9-]" (lower $trimmed) "-" ) | trimAll "-" | trunc 63 -}} + - name: config-services-volume-{{ $safeName }} mountPath: /configurations/services/{{ $value.name }} subPath: {{ $value.name }} readOnly: true {{- end }}
67-71: Keep volume names and referenced ConfigMap names in sync using the same $safeName.Prevents mismatch and ensures DNS-1123 compliance.
- {{- range $index, $value := .Values.beelzebubServiceConfigs }} - - name: config-services-volume-{{ trimSuffix ".yaml" $value.name }} + {{- range $index, $value := .Values.beelzebubServiceConfigs }} + {{- $trimmed := trimSuffix ".yaml" $value.name -}} + {{- $safeName := (regexReplaceAll "[^a-z0-9-]" (lower $trimmed) "-" ) | trimAll "-" | trunc 63 -}} + - name: config-services-volume-{{ $safeName }} configMap: - name: beelzebub-services-cm-{{ trimSuffix ".yaml" $value.name }} + name: beelzebub-services-cm-{{ $safeName }} {{- end }}Render and diff-check:
#!/bin/bash set -e helm template beelzebub ./beelzebub-chart > /tmp/r.yaml # Ensure every volume name has a matching ConfigMap name suffix awk '/- name: config-services-volume-/{print $4}' /tmp/r.yaml | sed 's/-$//' > /tmp/vn rg -n 'name:\s+beelzebub-services-cm-' /tmp/r.yaml | sed 's/.*cm-//' > /tmp/cn diff -u /tmp/vn /tmp/cn
🧹 Nitpick comments (1)
beelzebub-chart/templates/deployment.yaml (1)
16-19: Add config checksum annotations to trigger rolling updates on config changes.Without checksums, new ConfigMap data won’t roll pods automatically.
Outside this hunk, add under pod template metadata.annotations:
annotations: {{- range $v := .Values.beelzebubServiceConfigs }} {{- $trimmed := trimSuffix ".yaml" $v.name -}} {{- $safeName := (regexReplaceAll "[^a-z0-9-]" (lower $trimmed) "-" ) | trimAll "-" | trunc 63 -}} checksum/config-{{ $safeName }}: {{ toYaml $v.config | sha256sum }} {{- 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 (3)
beelzebub-chart/templates/configmap_services.yaml(1 hunks)beelzebub-chart/templates/deployment.yaml(2 hunks)beelzebub-chart/templates/service.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
beelzebub-chart/templates/service.yaml
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
beelzebub-chart/templates/configmap_services.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (2)
beelzebub-chart/templates/configmap_services.yaml (1)
1-2: Wrap therangein anifto guard against emptybeelzebubServiceConfigs.-{{- range $value := .Values.beelzebubServiceConfigs }} +{{- if .Values.beelzebubServiceConfigs }} +{{- range $value := .Values.beelzebubServiceConfigs }} … -{{- end }} +{{- end }} +{{- end }}Render locally with an empty
beelzebubServiceConfigslist to confirm no standalone---is emitted.beelzebub-chart/templates/service.yaml (1)
10-15: Extract numeric port and sanitize service port names
- Replace
trimPrefix ":"withregexFind "[0-9]+$"(fallback to.Values.service.port) to correctly parse addresses likehost:port.- Sanitize each ServicePort
nameas a valid DNS-1123 label: lowercase, replace non-alphanumerics with-, trim leading/trailing-, and truncate to 63 characters.- You don’t need to enforce unique numeric
portvalues—Kubernetes only requires each ServicePortnameto be unique.Likely an incorrect or invalid review comment.
| {{- range $value := .Values.beelzebubServiceConfigs }} | ||
| - name: {{ trimSuffix ".yaml" $value.name }} | ||
| containerPort: {{ trimPrefix ":" $value.config.address | default $.Values.service.port }} | ||
| protocol: TCP | ||
| {{- end }} |
There was a problem hiding this comment.
Robustly parse containerPort and sanitize port names.
Handle host:port and missing address; ensure containerPort is numeric; sanitize the name.
- {{- range $value := .Values.beelzebubServiceConfigs }}
- - name: {{ trimSuffix ".yaml" $value.name }}
- containerPort: {{ trimPrefix ":" $value.config.address | default $.Values.service.port }}
+ {{- range $value := .Values.beelzebubServiceConfigs }}
+ {{- $trimmed := trimSuffix ".yaml" $value.name -}}
+ {{- $safeName := (regexReplaceAll "[^a-z0-9-]" (lower $trimmed) "-" ) | trimAll "-" | trunc 63 -}}
+ {{- $raw := $value.config.address | default (printf ":%v" $.Values.service.port) -}}
+ {{- $portStr := regexFind "[0-9]+$" $raw | default (printf "%v" $.Values.service.port) -}}
+ {{- $port := $portStr | int -}}
+ - name: {{ $safeName }}
+ containerPort: {{ $port }}
protocol: TCP
- {{- end }}
+ {{- end }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{- range $value := .Values.beelzebubServiceConfigs }} | |
| - name: {{ trimSuffix ".yaml" $value.name }} | |
| containerPort: {{ trimPrefix ":" $value.config.address | default $.Values.service.port }} | |
| protocol: TCP | |
| {{- end }} | |
| {{- range $value := .Values.beelzebubServiceConfigs }} | |
| {{- $trimmed := trimSuffix ".yaml" $value.name -}} | |
| {{- $safeName := (regexReplaceAll "[^a-z0-9-]" (lower $trimmed) "-" ) | trimAll "-" | trunc 63 -}} | |
| {{- $raw := $value.config.address | default (printf ":%v" $.Values.service.port) -}} | |
| {{- $portStr := regexFind "[0-9]+$" $raw | default (printf "%v" $.Values.service.port) -}} | |
| {{- $port := $portStr | int -}} | |
| - name: {{ $safeName }} | |
| containerPort: {{ $port }} | |
| protocol: TCP | |
| {{- end }} |
🤖 Prompt for AI Agents
In beelzebub-chart/templates/deployment.yaml around lines 48-52, the port/name
templating must robustly handle host:port formats, missing addresses, ensure
containerPort is numeric, and sanitize the name: extract the port by splitting
on ":" and taking the last segment (so host:port -> port), fallback to
$.Values.service.port when address is empty, coerce the port to an integer (use
toInt or int conversion) and provide a safe default if conversion fails, and
sanitize the name by trimming suffix ".yaml" then lowercasing and replacing any
disallowed characters with "-" (use regexReplaceAll("[^a-z0-9-]", "-") and trim
leading/trailing "-") so the resulting name and containerPort are valid.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
beelzebub-chart/templates/configmap_services.yaml (2)
1-3: Good: per-item sanitization for DNS-1123 names.Deriving and reusing a sanitized
$safeNameper entry is the right move. Consider centralizing this logic in a named template helper (e.g., "beelzebub-chart.sanitizeName") to avoid duplication across templates.
4-12: ConfigMap data rendering looks correct; consider adding standard labels.Block scalar with
toYaml | nindent 4is valid for string data. Add chart labels for traceability and selectors consistency.Apply:
metadata: name: beelzebub-services-cm-{{ $safeName }} + labels: + {{- include "beelzebub-chart.labels" . | nindent 4 }}beelzebub-chart/templates/tests/test-connection.yaml (1)
11-16: Optional: pin BusyBox tag and add minimal retries.Pinning avoids pulling a moving target; simple retry improves test robustness.
- image: busybox + image: busybox:1.36 + # Optional: add backoff/retry + # command: ['sh','-c'] + # args: ['for i in 1 2 3; do wget -S -T3 {{ include "beelzebub-chart.fullname" $ }}:{{ $port }} && exit 0; sleep 2; done; exit 1']
📜 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 (4)
beelzebub-chart/templates/configmap_services.yaml(1 hunks)beelzebub-chart/templates/deployment.yaml(2 hunks)beelzebub-chart/templates/service.yaml(1 hunks)beelzebub-chart/templates/tests/test-connection.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- beelzebub-chart/templates/deployment.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
beelzebub-chart/templates/tests/test-connection.yaml
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
beelzebub-chart/templates/configmap_services.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
beelzebub-chart/templates/service.yaml
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
🔇 Additional comments (2)
beelzebub-chart/templates/service.yaml (1)
10-15: Verify Service port uniqueness: Duplicate numeric ports or sanitizednamevalues will prevent Service creation; please render the chart (e.g., withhelm template) and confirm allport:andname:entries under the Service are unique.beelzebub-chart/templates/tests/test-connection.yaml (1)
11-16: Cannot verify container names without Helm
Helm isn’t available in this environment—please runhelm template beelzebub ./beelzebub-chartlocally and confirm the rendered Pod spec has uniquename:entries for each container.
| {{- range $value := .Values.beelzebubServiceConfigs }} | ||
| - port: {{ trimPrefix ":" $value.config.address }} | ||
| targetPort: {{ trimPrefix ":" $value.config.address }} | ||
| protocol: TCP | ||
| name: http | ||
| name: {{ trimSuffix ".yaml" $value.name }} | ||
| {{- end }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address parsing is brittle; trimPrefix ":" fails for hosts like 0.0.0.0:8080.
Use a regex to extract the trailing port. Current rendering will break if address includes a host.
Included in the diff above via regexFind "[0-9]{2,5}$".
🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
🤖 Prompt for AI Agents
In beelzebub-chart/templates/service.yaml around lines 10 to 15, the current use
of trimPrefix ":" to get the port is brittle for addresses that include a host
(e.g. 0.0.0.0:8080); update the template to extract the trailing port with a
regex (e.g. use regexFind "[0-9]{2,5}$" on $value.config.address), use the regex
result for both port and targetPort, and add a safe fallback (default port or
skip the entry) if the regex returns empty so rendering won't fail.
Port name may violate IANA_SVC_NAME (length ≤15, lowercase, a-z start); sanitize and ensure uniqueness.
{{ trimSuffix ".yaml" $value.name }} can exceed 15 chars, include invalid chars, or start with a digit. Also, duplicates across entries will be rejected. Sanitize and cap to 15, and suffix with the loop index if needed.
Apply:
- ports:
- {{- range $value := .Values.beelzebubServiceConfigs }}
- - port: {{ trimPrefix ":" $value.config.address }}
- targetPort: {{ trimPrefix ":" $value.config.address }}
- protocol: TCP
- name: {{ trimSuffix ".yaml" $value.name }}
- {{- end }}
+ ports:
+ {{- range $i, $value := .Values.beelzebubServiceConfigs }}
+ {{- $trimmed := trimSuffix ".yaml" $value.name -}}
+ {{- $base := (regexReplaceAll "[^a-z0-9-]" (lower $trimmed) "-" ) | trimAll "-" -}}
+ {{- $portName := trunc 15 $base -}}
+ - port: {{ (regexFind "[0-9]{2,5}$" (toString $value.config.address)) | default (toString .Values.service.port) }}
+ targetPort: {{ (regexFind "[0-9]{2,5}$" (toString $value.config.address)) | default (toString .Values.service.port) }}
+ protocol: TCP
+ name: {{ $portName }}-{{ $i }}
+ {{- end }}Note: If you want to strictly enforce starting with a letter, prefix with p- when it doesn’t. I can provide a helper to do this cleanly.
🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
| {{- range $value := .Values.beelzebubServiceConfigs }} | ||
| - name: wget | ||
| image: busybox | ||
| command: ['wget'] | ||
| args: ['{{ include "beelzebub-chart.fullname" . }}:{{ .Values.service.port }}'] | ||
| args: ['{{ include "beelzebub-chart.fullname" . }}:{{ trimPrefix ":" $value.config.address }}'] | ||
| {{- end }} |
There was a problem hiding this comment.
Duplicate container names and wrong template scope inside range.
- All containers use
name: wget; container names must be unique. include "…fullname" .passes the loop item (not the root) due torange; use$for the root.- Address parsing should tolerate host:port (same concern as Service).
Apply:
- containers:
- {{- range $value := .Values.beelzebubServiceConfigs }}
- - name: wget
+ containers:
+ {{- range $i, $value := .Values.beelzebubServiceConfigs }}
+ {{- $trimmed := trimSuffix ".yaml" $value.name -}}
+ {{- $safeName := (regexReplaceAll "[^a-z0-9-]" (lower $trimmed) "-" ) | trimAll "-" | trunc 63 -}}
+ {{- $port := (regexFind "[0-9]{2,5}$" (toString $value.config.address)) | default (toString .Values.service.port) -}}
+ - name: wget-{{ $safeName }}-{{ $i }}
image: busybox
command: ['wget']
- args: ['{{ include "beelzebub-chart.fullname" . }}:{{ trimPrefix ":" $value.config.address }}']
+ args: ['{{ include "beelzebub-chart.fullname" $ }}:{{ $port }}']
{{- end }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{- range $value := .Values.beelzebubServiceConfigs }} | |
| - name: wget | |
| image: busybox | |
| command: ['wget'] | |
| args: ['{{ include "beelzebub-chart.fullname" . }}:{{ .Values.service.port }}'] | |
| args: ['{{ include "beelzebub-chart.fullname" . }}:{{ trimPrefix ":" $value.config.address }}'] | |
| {{- end }} | |
| containers: | |
| {{- range $i, $value := .Values.beelzebubServiceConfigs }} | |
| {{- $trimmed := trimSuffix ".yaml" $value.name -}} | |
| {{- $safeName := (regexReplaceAll "[^a-z0-9-]" (lower $trimmed) "-" ) | trimAll "-" | trunc 63 -}} | |
| {{- $port := (regexFind "[0-9]{2,5}$" (toString $value.config.address)) | default (toString .Values.service.port) -}} | |
| - name: wget-{{ $safeName }}-{{ $i }} | |
| image: busybox | |
| command: ['wget'] | |
| args: ['{{ include "beelzebub-chart.fullname" $ }}:{{ $port }}'] | |
| {{- end }} |
🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
🤖 Prompt for AI Agents
In beelzebub-chart/templates/tests/test-connection.yaml around lines 11-16, fix
duplicate container names and wrong template scope: make each container name
unique (e.g., append the loop index or a service identifier from the loop item),
stop using the loop-scope dot when calling the fullname template (use the root
context $ so: include "beelzebub-chart.fullname" $), and keep address parsing
tolerant of host:port by preserving the host:port portion while only trimming a
leading ":" if present (apply trimPrefix to the loop item’s address but do not
lose the host:port). Ensure the name generation and include use the correct
variables ($ and the loop index or item field) consistently.
|
Hi @sdibernardo, Nice to meet you, and thank you for your contribution to Beelzebub! I'll check it out in the next few days. 😄 Cheers, |
2 participants
All Submissions:
New Feature Submissions:
Summary by CodeRabbit