Merged
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances Helm chart flexibility by introducing standalone Redis architecture support and enabling customizable Ingress configurations for both Traefik and NGINX.
- Add
architecture: standaloneunder Redis values for single-pod scenarios - Expose
ingressClassNameandtypeinvalues.yamland supply separate Traefik/NGINX Ingress templates - Remove the generic ingress template, add a helper for
ingressClassName, and default the Secret name
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| charts/tooljet/values.yaml | Added architecture: standalone for Redis and new ingress keys |
| charts/tooljet/templates/traefik-ingress.yaml | New Ingress resource for Traefik-based routing |
| charts/tooljet/templates/nginx-ingress.yaml | New Ingress resource for NGINX-based routing |
| charts/tooljet/templates/ingress.yml | Removed generic Ingress template |
| charts/tooljet/templates/secret.yaml | Default fallback for Secret name |
| charts/tooljet/templates/_helpers.tpl | Helper function tooljet.ingressClassName |
Comments suppressed due to low confidence (4)
charts/tooljet/templates/traefik-ingress.yaml:11
- Rather than hardcoding
traefik, use the shared helper ({{ include "tooljet.ingressClassName" . }}) to respect any overrides invalues.yamland keep configuration consistent.
ingressClassName: traefik
charts/tooljet/templates/nginx-ingress.yaml:11
- Rather than hardcoding
nginx, leverage{{ include "tooljet.ingressClassName" . }}to ensure the ingress class remains in sync with the values file.
ingressClassName: nginx
charts/tooljet/values.yaml:122
- The default
tls: []entry was removed fromvalues.yaml. If TLS was previously enabled or expected, reintroduce an emptytlslist or document its removal to prevent unintentional disabling of TLS.
- tls: []
charts/tooljet/templates/_helpers.tpl:129
- Add unit tests for the
tooljet.ingressClassNamehelper to cover all branches (explicitingressClassName,nginx,traefik, fallback) and prevent regressions.
{{- define "tooljet.ingressClassName" -}}
Collaborator
Author
|
Updated ingress: Also removed duplicates from pgrst-deployment.yaml: |
adishM98
approved these changes
Jun 27, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Below are the updates been made for single redis pod and nginx-traefik compatibility.
architecture: standaloneline in values.yaml.values.yaml, Compatible with both Traefik or Nginx._helpers.tpl, added checks foringressClassNamefrom values.yaml.