Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 55 minutes and 25 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughHelm chart version bumped to 1.6.4. Deployment and Ingress templates now apply Helm Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
charts/kafka-ui/templates/deployment.yaml (1)
102-102: Quote templated resource names for YAML scalar safety and consistency.Line 94 already quotes templated
existingConfigMap; doing the same for Line 102, Line 171, and Line 176 avoids edge-case YAML coercion.♻️ Proposed refactor
- name: {{ tpl .Values.existingSecret . }} + name: {{ tpl .Values.existingSecret . | quote }} ... - name: {{ tpl .Values.yamlApplicationConfigConfigMap.name . }} + name: {{ tpl .Values.yamlApplicationConfigConfigMap.name . | quote }} ... - secretName: {{ tpl .Values.yamlApplicationConfigSecret.name . }} + secretName: {{ tpl .Values.yamlApplicationConfigSecret.name . | quote }}Also applies to: 171-171, 176-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/kafka-ui/templates/deployment.yaml` at line 102, The YAML scalar for templated resource names should be quoted to avoid coercion issues: update the occurrences that use the tpl helper with .Values.existingSecret (and the other two occurrences noted for existingConfigMap/existingSecret) so they are rendered as quoted strings; for example change name: {{ tpl .Values.existingSecret . }} to name: "{{ tpl .Values.existingSecret . }}" and do the same for the other templated instances (the tpl usages around existingConfigMap and existingSecret at the referenced locations).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/kafka-ui/templates/deployment.yaml`:
- Around line 70-72: The Helm template pipelines call tpl before default which
can fail when keyName is unset; change the pipelines to apply default first then
pass that to tpl so the fallback "config.yml" is used: for both occurrences
referencing .Values.yamlApplicationConfigConfigMap.keyName and
.Values.yamlApplicationConfigSecret.keyName, wrap each keyName with default
"config.yml" (i.e. default .Values...keyName "config.yml") and then call tpl on
that result instead of tpl on the raw keyName.
- Around line 160-162: The volumes block misuses tpl on the object
.Values.volumes which stringifies it and breaks YAML; change the block to mirror
the env pattern (use with .Values.volumes and pass the object directly to toYaml
then nindent) instead of applying tpl — update the template surrounding
.Values.volumes so it uses {{- with .Values.volumes }} then {{- toYaml . |
nindent 8 }} and {{- end }}, leaving tpl only for actual template strings.
---
Nitpick comments:
In `@charts/kafka-ui/templates/deployment.yaml`:
- Line 102: The YAML scalar for templated resource names should be quoted to
avoid coercion issues: update the occurrences that use the tpl helper with
.Values.existingSecret (and the other two occurrences noted for
existingConfigMap/existingSecret) so they are rendered as quoted strings; for
example change name: {{ tpl .Values.existingSecret . }} to name: "{{ tpl
.Values.existingSecret . }}" and do the same for the other templated instances
(the tpl usages around existingConfigMap and existingSecret at the referenced
locations).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1aaecda-e71b-4057-8335-5bd4c2553eba
📒 Files selected for processing (2)
charts/kafka-ui/Chart.yamlcharts/kafka-ui/templates/deployment.yaml
Haarolean
left a comment
Haarolean
left a comment
There was a problem hiding this comment.
please address coderabbit comments
I'll have a look over the next few days. (I likely need to test the issues and fixes) |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/kafka-ui/templates/deployment.yaml`:
- Around line 100-102: The templated secret name produced by tpl
(.Values.existingSecret .) is emitted as an unquoted YAML scalar which can be
mis-parsed for values like "on"/"yes"/"true"; update the deployment template to
emit a quoted string by wrapping the tpl call with Helm's quote function (i.e.,
use quote (tpl .Values.existingSecret .)) for the secretRef.name so the rendered
name is always a proper YAML string; mirror the same quoting approach used for
existingConfigMap to ensure consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a0959b64-2cb9-4d65-b6b7-07c3a1b09659
📒 Files selected for processing (2)
charts/kafka-ui/Chart.yamlcharts/kafka-ui/templates/deployment.yaml
✅ Files skipped from review due to trivial changes (1)
- charts/kafka-ui/Chart.yaml
|
I'm not sure where to view the requested changes. (I did rebase on main to get rid of the conflicts, not sure if that got rid of them or if it was just the CodeRabbit issues) |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
I thought the edit did not trigger it, deleted and then posted the correct message again, so probably hitting the rate limit again... Need to learn to refresh the issue to be sure I see the latest comments. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/kafka-ui/templates/deployment.yaml`:
- Around line 173-181: The template currently calls tpl on
.Values.yamlApplicationConfigConfigMap.name and
.Values.yamlApplicationConfigSecret.name without validating presence; update
those tpl calls to use the required function around the name keys so Helm will
emit clear errors if name is missing (match the pattern already used elsewhere
in this file for yamlApplicationConfigConfigMap/name and
yamlApplicationConfigSecret/name), keeping the surrounding keys
(kafka-ui-yaml-conf-configmap and kafka-ui-yaml-conf-secret) intact and quoting
the result as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccb3aa4c-0c5e-4c4c-9b8a-22da09a82341
📒 Files selected for processing (2)
charts/kafka-ui/templates/deployment.yamlcharts/kafka-ui/templates/ingress.yaml
✅ Files skipped from review due to trivial changes (1)
- charts/kafka-ui/templates/ingress.yaml
| {{- if .Values.yamlApplicationConfigConfigMap}} | ||
| - name: kafka-ui-yaml-conf-configmap | ||
| configMap: | ||
| name: {{ .Values.yamlApplicationConfigConfigMap.name }} | ||
| configMap: | ||
| name: {{ tpl .Values.yamlApplicationConfigConfigMap.name . | quote }} | ||
| {{- end }} | ||
| {{- if .Values.yamlApplicationConfigSecret}} | ||
| - name: kafka-ui-yaml-conf-secret | ||
| secret: | ||
| secretName: {{ .Values.yamlApplicationConfigSecret.name }} | ||
| secretName: {{ tpl .Values.yamlApplicationConfigSecret.name . | quote }} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Helm templates, does calling tpl with a missing value (nil/) fail, and is wrapping the value with requiredbeforetpl the recommended way to surface a clear error for required string fields?
💡 Result:
Yes, calling tpl with a missing value (nil or ) fails the template rendering, typically with a "nil pointer evaluating interface" error or tpl execution error. Wrapping the value with required before passing to tpl is the recommended way to surface a clear, custom error for required string fields, as per official Helm documentation.
Citations:
- 1: https://helm.sh/docs/howto/charts_tips_and_tricks
- 2: https://stackoverflow.com/questions/65790905/nil-pointer-evaluating-interface-when-installing-a-helm-chart
- 3: https://helm.sh/docs/chart_template_guide/functions_and_pipelines
- 4: https://stackoverflow.com/questions/53866196/how-best-to-say-a-value-is-required-in-a-helm-chart
- 5: https://helm.sh/docs/howto/charts_tips_and_tricks/
- 6: https://helm.sh/docs/chart%5Ftemplate%5Fguide/function%5Flist/
🏁 Script executed:
# Check if the file exists and read lines 173-181
cat -n charts/kafka-ui/templates/deployment.yaml | sed -n '170,185p'Repository: kafbat/helm-charts
Length of output: 794
🏁 Script executed:
# Search for existing uses of `required` in this file to understand existing patterns
grep -n "required" charts/kafka-ui/templates/deployment.yamlRepository: kafbat/helm-charts
Length of output: 564
🏁 Script executed:
# Check for similar configMap/secret patterns in other templates
fd -e yaml charts/kafka-ui/templates/ | xargs grep -l "configMap\|secretName" | head -5Repository: kafbat/helm-charts
Length of output: 471
🏁 Script executed:
# Search for similar patterns in other templates using correct fd syntax
fd . charts/kafka-ui/templates/ --type f -e yaml | xargs grep -n "tpl.*\.name" | grep -v "required"Repository: kafbat/helm-charts
Length of output: 320
Add required guards before tpl to provide clear error messages for missing ConfigMap/Secret names.
Lines 176 and 181 pass .name values to tpl without validation. If either yamlApplicationConfigConfigMap or yamlApplicationConfigSecret is present without a name field, the template fails with a generic rendering error. The same file already uses this pattern successfully at lines 79, 80, 86, and 87. Align these lines with the existing pattern.
Suggested fix
{{- if .Values.yamlApplicationConfigConfigMap}}
- name: kafka-ui-yaml-conf-configmap
configMap:
- name: {{ tpl .Values.yamlApplicationConfigConfigMap.name . | quote }}
+ name: {{ tpl (required "Missing required value yamlApplicationConfigConfigMap.name" .Values.yamlApplicationConfigConfigMap.name) . | quote }}
{{- end }}
{{- if .Values.yamlApplicationConfigSecret}}
- name: kafka-ui-yaml-conf-secret
secret:
- secretName: {{ tpl .Values.yamlApplicationConfigSecret.name . | quote }}
+ secretName: {{ tpl (required "Missing required value yamlApplicationConfigSecret.name" .Values.yamlApplicationConfigSecret.name) . | quote }}
{{- 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.
| {{- if .Values.yamlApplicationConfigConfigMap}} | |
| - name: kafka-ui-yaml-conf-configmap | |
| configMap: | |
| name: {{ .Values.yamlApplicationConfigConfigMap.name }} | |
| configMap: | |
| name: {{ tpl .Values.yamlApplicationConfigConfigMap.name . | quote }} | |
| {{- end }} | |
| {{- if .Values.yamlApplicationConfigSecret}} | |
| - name: kafka-ui-yaml-conf-secret | |
| secret: | |
| secretName: {{ .Values.yamlApplicationConfigSecret.name }} | |
| secretName: {{ tpl .Values.yamlApplicationConfigSecret.name . | quote }} | |
| {{- if .Values.yamlApplicationConfigConfigMap}} | |
| - name: kafka-ui-yaml-conf-configmap | |
| configMap: | |
| name: {{ tpl (required "Missing required value yamlApplicationConfigConfigMap.name" .Values.yamlApplicationConfigConfigMap.name) . | quote }} | |
| {{- end }} | |
| {{- if .Values.yamlApplicationConfigSecret}} | |
| - name: kafka-ui-yaml-conf-secret | |
| secret: | |
| secretName: {{ tpl (required "Missing required value yamlApplicationConfigSecret.name" .Values.yamlApplicationConfigSecret.name) . | quote }} | |
| {{- end }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/kafka-ui/templates/deployment.yaml` around lines 173 - 181, The
template currently calls tpl on .Values.yamlApplicationConfigConfigMap.name and
.Values.yamlApplicationConfigSecret.name without validating presence; update
those tpl calls to use the required function around the name keys so Helm will
emit clear errors if name is missing (match the pattern already used elsewhere
in this file for yamlApplicationConfigConfigMap/name and
yamlApplicationConfigSecret/name), keeping the surrounding keys
(kafka-ui-yaml-conf-configmap and kafka-ui-yaml-conf-secret) intact and quoting
the result as before.
There was a problem hiding this comment.
That issue was present in the chart before this PR?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the git log for the original state of these lines before this PR
git log --oneline -5
git show HEAD~1:charts/kafka-ui/templates/deployment.yaml 2>/dev/null | sed -n '160,185p' || \
git log --oneline --all | head -5
Length of output: 1369
🏁 Script executed:
#!/bin/bash
# Check the base branch (main) state of the file for these lines
git fetch origin main 2>/dev/null
git show origin/main:charts/kafka-ui/templates/deployment.yaml 2>/dev/null | grep -n "yamlApplicationConfigConfigMap\|yamlApplicationConfigSecret\|required\|tpl" | head -30
Length of output: 1774
@mohag — Partially, yes. On origin/main before this PR, those lines were:
name: {{ .Values.yamlApplicationConfigConfigMap.name }}
secretName: {{ .Values.yamlApplicationConfigSecret.name }}— no tpl and no required. The lack of a required guard was pre-existing.
However, this PR introduces the tpl call on those values, which changes the failure mode:
- Before this PR: a missing
.namesilently renders as<no value>in the YAML, producing a malformed but quietly broken manifest. - After this PR (current state):
tplon anilvalue causes a hard template rendering error with a generic message.
Since the PR is already touching these lines to add tpl, it's a natural opportunity to also add required to match the pattern already used elsewhere in the same file for envs.secretMappings and envs.configMappings (lines 79, 80, 86, 87). That would give users a clear, actionable error message instead of a cryptic one.
2 participants
Template the config settings that can be used to configure the cluster that kafka-ui talks to.
Happened to clean up a bit of trailing whitespace.
Partially addresses #52. (the parts that I need and a few similar settings)
Summary by CodeRabbit
Bug Fixes
Chores