Conversation
📝 WalkthroughWalkthroughThe pull request adds Ingress support to the cluster-api-visualizer Helm chart. A new Ingress template was created with conditional rendering, while configuration values were added to enable optional TLS, hostname, and class settings. The chart version was bumped to 1.6.0. 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: 1
🧹 Nitpick comments (1)
helm/cluster-api-visualizer/templates/ingress.yaml (1)
29-29: ConsiderPrefixinstead ofImplementationSpecificfor predictable cross-controller behavior.
ImplementationSpecificleaves path matching semantics to the controller. NGINX Ingress treats it as equivalent toPrefixby default, but Traefik interprets it as flexible string prefix matching (unlike strictPrefix). For consistent, spec-compliant behavior across NGINX, Traefik, and other controllers, usePrefixfor root path/.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/cluster-api-visualizer/templates/ingress.yaml` at line 29, Change the Ingress pathType from ImplementationSpecific to Prefix for the root path to ensure consistent, spec-compliant prefix matching across controllers; locate the Ingress resource where the pathType field is set (look for the path with "/" in the ingress.yaml template) and replace the value "ImplementationSpecific" with "Prefix" so controllers like NGINX and Traefik will behave predictably.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@helm/cluster-api-visualizer/templates/ingress.yaml`:
- Line 5: The templates currently hardcode the resource name "capi-visualizer",
causing collisions across releases; replace those hardcoded names (e.g., the
name field in ingress.yaml and the other templates that reference
"capi-visualizer") with a release-aware helper: add a fullname helper in
_helpers.tpl (or use {{ .Release.Name }}-capi-visualizer) and reference that
helper consistently across ingress.yaml (both occurrences), service.yaml,
deployment.yaml, clusterrole.yaml, clusterrolebinding.yaml, and
serviceaccount.yaml so every resource name uses the release-prefixed fullname
helper instead of the literal "capi-visualizer".
---
Nitpick comments:
In `@helm/cluster-api-visualizer/templates/ingress.yaml`:
- Line 29: Change the Ingress pathType from ImplementationSpecific to Prefix for
the root path to ensure consistent, spec-compliant prefix matching across
controllers; locate the Ingress resource where the pathType field is set (look
for the path with "/" in the ingress.yaml template) and replace the value
"ImplementationSpecific" with "Prefix" so controllers like NGINX and Traefik
will behave predictably.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dce007b4-a16e-42e0-a6c9-1519012ba17f
📒 Files selected for processing (3)
helm/cluster-api-visualizer/Chart.yamlhelm/cluster-api-visualizer/templates/ingress.yamlhelm/cluster-api-visualizer/values.yaml
| apiVersion: networking.k8s.io/v1 | ||
| kind: Ingress | ||
| metadata: | ||
| name: capi-visualizer |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify hardcoded naming usage and helper availability in chart templates (read-only).
rg -n -C2 'name:\s*capi-visualizer|serviceAccountName:\s*capi-visualizer' helm/cluster-api-visualizer/templates
fd '_helpers.tpl' helm/cluster-api-visualizer -x sed -n '1,220p' {}Repository: Jont828/cluster-api-visualizer
Length of output: 4103
🏁 Script executed:
fd '_helpers.tpl' helm/cluster-api-visualizerRepository: Jont828/cluster-api-visualizer
Length of output: 56
🏁 Script executed:
ls -la helm/cluster-api-visualizer/templates/
cat -n helm/cluster-api-visualizer/values.yamlRepository: Jont828/cluster-api-visualizer
Length of output: 1154
🏁 Script executed:
cat -n helm/cluster-api-visualizer/templates/ingress.yaml
cat -n helm/cluster-api-visualizer/templates/service.yamlRepository: Jont828/cluster-api-visualizer
Length of output: 1589
🏁 Script executed:
cat -n helm/cluster-api-visualizer/templates/deployment.yaml
cat -n helm/cluster-api-visualizer/templates/clusterrole.yamlRepository: Jont828/cluster-api-visualizer
Length of output: 2084
Replace hardcoded capi-visualizer names with release-aware templating to support multi-release deployments.
Hardcoded resource names prevent proper release isolation. Multiple releases of this chart will collide on the same resource names (Ingress, Service, Deployment, ServiceAccount, ClusterRole, ClusterRoleBinding all hardcode capi-visualizer).
Use {{ .Release.Name }}-capi-visualizer or create a _helpers.tpl with a fullname template helper (standard Helm pattern) and reference it consistently across all templates. Apply to ingress.yaml (lines 5, 32), service.yaml, deployment.yaml, clusterrole.yaml, clusterrolebinding.yaml, and serveaccount.yaml.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@helm/cluster-api-visualizer/templates/ingress.yaml` at line 5, The templates
currently hardcode the resource name "capi-visualizer", causing collisions
across releases; replace those hardcoded names (e.g., the name field in
ingress.yaml and the other templates that reference "capi-visualizer") with a
release-aware helper: add a fullname helper in _helpers.tpl (or use {{
.Release.Name }}-capi-visualizer) and reference that helper consistently across
ingress.yaml (both occurrences), service.yaml, deployment.yaml,
clusterrole.yaml, clusterrolebinding.yaml, and serviceaccount.yaml so every
resource name uses the release-prefixed fullname helper instead of the literal
"capi-visualizer".
There was a problem hiding this comment.
that would be a good idea, but it's currently not implemented in the chart ;-)
There was a problem hiding this comment.
@Nold360, fair point — the hardcoded capi-visualizer naming is a pre-existing pattern across the whole chart (Deployment, Service, ClusterRole, etc.), so the new ingress template is simply following the established convention. This PR isn't the right place to refactor all of that.
Would you like me to open a follow-up GitHub issue to track adding a _helpers.tpl with a release-aware fullname helper and updating all templates consistently? That way it doesn't get lost.
1 participant
Simple ingress addition to the helm chart.