Create new HTTPRoute template & update values file#62
Create new HTTPRoute template & update values file#62attributeerror wants to merge 2 commits intokafbat:mainfrom
Conversation
attributeerror
force-pushed
the
add-support-for-gateway-api
branch
from
6c3e054 to
03df0e0
Compare
|
we're moving from ingress-nginx to gateway-api and would be great to have this supported by the chart |
|
We are also interested in getting httproute support! |
|
We need this gateway-api☝️ |
This comment has been minimized.
This comment has been minimized.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdded a new Helm template for Kubernetes Gateway API Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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)
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 (2)
charts/kafka-ui/templates/http-route.yaml (2)
13-14: Consider name length limits.Kubernetes resource names are limited to 63 characters. If
$.Chart.Namecombined with the gateway name and-httproutesuffix exceeds this limit, the deployment will fail. Consider usingtrunc 63 | trimSuffix "-"similar to thekafka-ui.fullnamehelper.💡 Proposed fix to handle name length
- name: {{ tpl ($route.nameOverride | default (printf "%s-%s" $.Chart.Name (first $route.gatewayParentRefs).name)) $ }}-httproute + name: {{ tpl ($route.nameOverride | default (printf "%s-%s" $.Chart.Name (first $route.gatewayParentRefs).name)) $ | trunc 54 | trimSuffix "-" }}-httproute🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/kafka-ui/templates/http-route.yaml` around lines 13 - 14, The metadata name construction for the HTTPRoute can exceed Kubernetes' 63-character limit; update the template that builds the name (the line using tpl with ($route.nameOverride | default (printf "%s-%s" $.Chart.Name (first $route.gatewayParentRefs).name)) and the "-httproute" suffix) to truncate the final name to 63 characters and remove a trailing dash (use Helm template functions like trunc 63 | trimSuffix "-" similar to the kafka-ui.fullname helper) so the generated resource name never violates the length constraint.
72-77: Minor edge case with document separators.When enabled routes are followed by disabled routes, a trailing
---separator may be generated. This is technically valid YAML, but if you want to be precise, the separator logic could account for whether subsequent routes are enabled. This is a minor cosmetic issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/kafka-ui/templates/http-route.yaml` around lines 72 - 77, The current document-separator logic uses lt (add1 $routeIdx) (len $.Values.httpRoutes) and may emit a trailing '---' when remaining routes are all disabled; update the condition around the '---' to check whether there exists any subsequent route with .enabled true instead of just comparing indices: from the block that references $routeIdx and $.Values.httpRoutes, iterate (or use a helper) over the slice of routes after $routeIdx to detect any enabled route and only render the '---' if one is found, leaving the rest of the template unchanged.
🤖 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/http-route.yaml`:
- Around line 37-38: The hostnames block currently always renders and may
produce invalid YAML when $route.hostnames is empty or undefined; update the
template to conditionally render the hostnames section only when
$route.hostnames is set and non-empty (e.g., wrap the existing line using an if
test like "if $route.hostnames" or "if and $route.hostnames (gt (len
$route.hostnames) 0)" and keep the existing "{{- tpl (toYaml $route.hostnames) $
| nindent 2 }}" inside that block) so that hostnames is omitted entirely when
there are no values.
- Around line 42-49: Replace the tab-indented, tpl-wrapped port line with a
space-indented, non-tpl expression that treats port as an integer; specifically,
in the http-route.yaml backendRefs block (the section guarded by {{- if
$route.shouldRouteToService }} and referencing include "kafka-ui.fullname" and
$.Values.service.port) remove the tpl call and the tab, then render the port as
an integer, e.g. use a spaces-indented line like: port: {{ $.Values.service.port
| default 80 | int }} so the template no longer calls tpl on an int and the YAML
indentation uses spaces.
---
Nitpick comments:
In `@charts/kafka-ui/templates/http-route.yaml`:
- Around line 13-14: The metadata name construction for the HTTPRoute can exceed
Kubernetes' 63-character limit; update the template that builds the name (the
line using tpl with ($route.nameOverride | default (printf "%s-%s" $.Chart.Name
(first $route.gatewayParentRefs).name)) and the "-httproute" suffix) to truncate
the final name to 63 characters and remove a trailing dash (use Helm template
functions like trunc 63 | trimSuffix "-" similar to the kafka-ui.fullname
helper) so the generated resource name never violates the length constraint.
- Around line 72-77: The current document-separator logic uses lt (add1
$routeIdx) (len $.Values.httpRoutes) and may emit a trailing '---' when
remaining routes are all disabled; update the condition around the '---' to
check whether there exists any subsequent route with .enabled true instead of
just comparing indices: from the block that references $routeIdx and
$.Values.httpRoutes, iterate (or use a helper) over the slice of routes after
$routeIdx to detect any enabled route and only render the '---' if one is found,
leaving the rest of the template unchanged.
🪄 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: 18e23c98-a89b-4e37-8b45-a78d1c1ab112
📒 Files selected for processing (2)
charts/kafka-ui/templates/http-route.yamlcharts/kafka-ui/values.yaml
| hostnames: | ||
| {{- tpl (toYaml $route.hostnames) $ | nindent 2 }} |
There was a problem hiding this comment.
Handle empty or undefined hostnames gracefully.
If hostnames is undefined or an empty array, toYaml will produce null or [], which may cause invalid YAML. Consider making the hostnames field conditional or adding validation.
💡 Proposed fix to make hostnames conditional
+ {{- if $route.hostnames }}
hostnames:
{{- tpl (toYaml $route.hostnames) $ | nindent 2 }}
+ {{- 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.
| hostnames: | |
| {{- tpl (toYaml $route.hostnames) $ | nindent 2 }} | |
| {{- if $route.hostnames }} | |
| hostnames: | |
| {{- tpl (toYaml $route.hostnames) $ | nindent 2 }} | |
| {{- end }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/kafka-ui/templates/http-route.yaml` around lines 37 - 38, The
hostnames block currently always renders and may produce invalid YAML when
$route.hostnames is empty or undefined; update the template to conditionally
render the hostnames section only when $route.hostnames is set and non-empty
(e.g., wrap the existing line using an if test like "if $route.hostnames" or "if
and $route.hostnames (gt (len $route.hostnames) 0)" and keep the existing "{{-
tpl (toYaml $route.hostnames) $ | nindent 2 }}" inside that block) so that
hostnames is omitted entirely when there are no values.
| {{- if $route.shouldRouteToService }} | ||
| backendRefs: | ||
| - group: '' | ||
| kind: Service | ||
| name: {{ include "kafka-ui.fullname" $ }} | ||
| port: {{ (tpl (default "80" ($.Values.service.port)) $) | int }} | ||
| weight: 1 | ||
| {{- end }} |
There was a problem hiding this comment.
Critical: Template execution failure and indentation error on port line.
Two issues on line 47:
-
Tab character: Line 47 uses a tab for indentation instead of spaces, which will cause YAML parsing errors in the rendered output.
-
tplon integer:$.Values.service.portis an integer (default80). Thetplfunction expects a string, so this will fail with "expected string; found int" when the template executes.
Since port values typically don't need Go template interpolation, tpl is unnecessary here.
🐛 Proposed fix
backendRefs:
- group: ''
kind: Service
name: {{ include "kafka-ui.fullname" $ }}
- port: {{ (tpl (default "80" ($.Values.service.port)) $) | int }}
+ port: {{ $.Values.service.port | default 80 | int }}
weight: 1📝 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 $route.shouldRouteToService }} | |
| backendRefs: | |
| - group: '' | |
| kind: Service | |
| name: {{ include "kafka-ui.fullname" $ }} | |
| port: {{ (tpl (default "80" ($.Values.service.port)) $) | int }} | |
| weight: 1 | |
| {{- end }} | |
| {{- if $route.shouldRouteToService }} | |
| backendRefs: | |
| - group: '' | |
| kind: Service | |
| name: {{ include "kafka-ui.fullname" $ }} | |
| port: {{ $.Values.service.port | default 80 | int }} | |
| weight: 1 | |
| {{- end }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/kafka-ui/templates/http-route.yaml` around lines 42 - 49, Replace the
tab-indented, tpl-wrapped port line with a space-indented, non-tpl expression
that treats port as an integer; specifically, in the http-route.yaml backendRefs
block (the section guarded by {{- if $route.shouldRouteToService }} and
referencing include "kafka-ui.fullname" and $.Values.service.port) remove the
tpl call and the tab, then render the port as an integer, e.g. use a
spaces-indented line like: port: {{ $.Values.service.port | default 80 | int }}
so the template no longer calls tpl on an int and the YAML indentation uses
spaces.
Ports kafbat#62 with all CodeRabbit review fixes applied: - Truncate resource name to 63-char limit (trunc 54 + "-httproute" suffix) - Conditionally render hostnames block only when non-empty - Fix tab indentation and remove tpl wrapping on service port (render as int) - Fix trailing --- separator to only emit when a subsequent enabled route exists Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
i see this has a lot of upvotes, happy to merge if someone can take a look at coderabbit notes in a separate PR |
6 participants
Add support for deploying HTTPRoute resources, as well as Ingress. This allows teams making use of the Gateway API to expose kafka-ui via this chart.
Summary by CodeRabbit
New Features