feat: add form-waiting and form-test paths to ingress routing #268
Conversation
WalkthroughThis pull request updates the n8n Helm chart from version 2.0.1 to 2.0.2 and adds two new ingress routes ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-09-02T08:05:39.372ZApplied to files:
🔇 Additional comments (2)
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 |
babu97
left a comment
babu97
left a comment
There was a problem hiding this comment.
Looks good for me, it hasn't broken dev instance
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
101-140: Fix YAML indentation in the ingress example (currently inconsistent / likely invalid on copy-paste).
paths:is nested under the- host:item; thepaths:key should align withhost:under the same list item. Also consider includingservice/portonly if that’s actually configurable in values (don’t imply it is).ingress: enabled: false annotations: {} # define a custom ingress class Name, like "traefik" or "nginx" className: "" hosts: - host: workflow.example.com - paths: - - path: / - pathType: Prefix + paths: + - path: / + pathType: Prefix tls: - hosts: - workflow.example.com secretName: host-domain-cert # ... next n8n specific section
🧹 Nitpick comments (2)
charts/n8n/values.yaml (1)
35-45: Breaking values change:ingress.hosts[].pathsis now objects; ensure migration is documented.
Defaults are good, but existing users withpaths: ["/"]will render invalid templates unless they update values (major chart bump helps, but a clear README migration note prevents surprises).README.md (1)
787-799: Valkey section: good direction; consider clarifying “replaces Redis section” to prevent confusion.
The header and link help, but readers coming from older chart versions may not realize howvalkey:relates to priorredis:settings and/or external Redis. A one-liner here would reduce migration friction.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
charts/n8n/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.github/workflows/lint-test.yaml(1 hunks)README.md(4 hunks)charts/n8n/Chart.yaml(2 hunks)charts/n8n/templates/NOTES.txt(1 hunks)charts/n8n/templates/ingress.yaml(1 hunks)charts/n8n/values.yaml(2 hunks)examples/README.md(1 hunks)examples/values_full.yaml(3 hunks)examples/values_small_prod.yaml(1 hunks)helmfile.yaml(0 hunks)
💤 Files with no reviewable changes (1)
- helmfile.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T08:05:39.372Z
Learnt from: tsch157
Repo: 8gears/n8n-helm-chart PR: 239
File: charts/n8n/Chart.yaml:3-4
Timestamp: 2025-09-02T08:05:39.372Z
Learning: n8n 1.110.0 is a pre-release version and should not be used in production deployments. Always verify if a version is stable before recommending upgrades in the n8n Helm chart.
Applied to files:
charts/n8n/Chart.yaml
🪛 LanguageTool
README.md
[grammar] ~145-~145: Use a hyphen to join words.
Context: ...oned in the excerpt below. Treat the n8n provided configuration documentation as ...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (10)
examples/README.md (1)
30-33: Good update to a newer CloudNativePG manifest URL, but consider pinning guidance to avoid silent drift.
Using a fixed release asset is good; you may also want to mention (in text) that users should pick the CNPG release matching their cluster / operator version expectations.examples/values_small_prod.yaml (1)
61-65: Ingress example updated correctly to the new{path, pathType}shape.
This is consistent with the updated ingress template.charts/n8n/templates/NOTES.txt (1)
3-6: NOTES rendering matches the new ingress paths object model.
LGTM.examples/values_full.yaml (3)
39-42: Queue Redis host update looks consistent with the Valkey subchart rename.
103-108: Ingress example updated correctly to includepathType.
81-92: Valkey example values confirmed to match chart 0.8.1 specification.The
replicaCount,auth.enabled, anddataStorage.enabledvalues all match the chart defaults. Theresources.requestsare custom overrides that align with the chart's documented example pattern (the chart itself defaults to empty resources with commented examples).charts/n8n/values.yaml (1)
686-697: Valkey section rename is clear; please ensure README uses the same terminology consistently.charts/n8n/templates/ingress.yaml (1)
33-41: Ingress per-path fields look correct and consistent with examples/NOTES.charts/n8n/Chart.yaml (2)
3-4: No action required. TheappVersion: 1.122.4is a stable release and safe for production use.
27-31: Valkey dependency v0.8.1 confirmed with correct repository and expected values schema.The chart contains all expected top-level properties:
replicaCount,auth, anddataStorage. The repository URL (https://valkey.io/valkey-helm/) is correct. The values schema is properly structured and ready for use.
| - name: Add Helm Repositories | ||
| run: | | ||
| helm repo add bitnami https://charts.bitnami.com/bitnami | ||
| helm repo add valkey https://valkey.io/valkey-helm/ | ||
| helm repo update | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Valkey Helm repository https://valkey.io/valkey-helm/ official documented
💡 Result:
The official Valkey Helm repository page (with usage docs) is hosted at https://valkey.io/valkey-helm/ — add with:
helm repo add valkey https://valkey.io/valkey-helm/
helm repo update
helm install valkey valkey/valkey. [1]
The chart source and full README/releases (valkey-io/valkey-helm) are on GitHub: https://github.com/valkey-io/valkey-helm. [2]
Sources:
[1] Valkey Helm Charts — https://valkey.io/valkey-helm/
[2] valkey-io/valkey-helm (GitHub README & releases) — https://github.com/valkey-io/valkey-helm
Acknowledge the external dependency risk for CI stability.
The Valkey Helm repository URL (https://valkey.io/valkey-helm/) is official and properly documented. However, relying on this external endpoint means ct install/lint may intermittently fail if the repository becomes unavailable. Consider caching the repo index or using mirror strategies to improve CI reliability.
| {{- if $.Values.webhook.enabled }} | ||
| - path: {{ . }}webhook/ | ||
| pathType: Prefix | ||
| - path: {{ trimSuffix "/" .path }}/webhook/ | ||
| pathType: {{ .pathType }} | ||
| backend: | ||
| service: | ||
| name: {{ $fullName }}-webhook | ||
| port: | ||
| number: {{ $.Values.webhook.service.port | default 80 }} | ||
| # Note: The default URL for manual workflow executions is /webhook-test/*. Make sure that these URLs route to your main process. | ||
| - path: {{ . }}webhook-test/ | ||
| pathType: Prefix | ||
| - path: {{ trimSuffix "/" .path }}/webhook-test/ | ||
| pathType: {{ .pathType }} | ||
| backend: | ||
| service: | ||
| name: {{ $fullName }} | ||
| port: | ||
| number: {{ $.Values.main.service.port | default 80 }} | ||
| - path: {{ . }}webhook-waiting/ | ||
| pathType: Prefix | ||
| number: {{ $.Values.main.service.port | default 80 }} | ||
| - path: {{ trimSuffix "/" .path }}/webhook-waiting/ | ||
| pathType: {{ .pathType }} | ||
| backend: | ||
| service: | ||
| name: {{ $fullName }}-webhook | ||
| port: | ||
| number: {{ $.Values.webhook.service.port | default 80 }} | ||
| - path: {{ trimSuffix "/" .path }}/form/ | ||
| pathType: {{ .pathType }} | ||
| backend: | ||
| service: | ||
| name: {{ $fullName }}-webhook | ||
| port: | ||
| number: {{ $.Values.webhook.service.port | default 80 }} | ||
| - path: {{ . }}form/ | ||
| pathType: Prefix | ||
| - path: {{ trimSuffix "/" .path }}/form-waiting/ | ||
| pathType: {{ .pathType }} | ||
| backend: | ||
| service: | ||
| name: {{ $fullName }}-webhook | ||
| port: | ||
| number: {{ $.Values.webhook.service.port | default 80 }} | ||
| - path: {{ trimSuffix "/" .path }}/form-test/ | ||
| pathType: {{ .pathType }} | ||
| backend: | ||
| service: | ||
| name: {{ $fullName }} | ||
| port: | ||
| number: {{ $.Values.main.service.port | default 80 }} | ||
| {{- end }} |
There was a problem hiding this comment.
Potential routing bug if users set pathType: Exact on base paths (derived subpaths will stop matching).
Right now /webhook/, /webhook-test/, /webhook-waiting/, /form* inherit .pathType. For correctness/safety, consider hardcoding Prefix for these derived subpaths (or explicitly validating/documenting that base pathType must be Prefix).
{{- if $.Values.webhook.enabled }}
- path: {{ trimSuffix "/" .path }}/webhook/
- pathType: {{ .pathType }}
+ pathType: Prefix
...
- path: {{ trimSuffix "/" .path }}/webhook-test/
- pathType: {{ .pathType }}
+ pathType: Prefix
...
- path: {{ trimSuffix "/" .path }}/webhook-waiting/
- pathType: {{ .pathType }}
+ pathType: Prefix
...
- path: {{ trimSuffix "/" .path }}/form/
- pathType: {{ .pathType }}
+ pathType: Prefix
...
- path: {{ trimSuffix "/" .path }}/form-waiting/
- pathType: {{ .pathType }}
+ pathType: Prefix
...
- path: {{ trimSuffix "/" .path }}/form-test/
- pathType: {{ .pathType }}
+ pathType: Prefix
{{- 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.webhook.enabled }} | |
| - path: {{ . }}webhook/ | |
| pathType: Prefix | |
| - path: {{ trimSuffix "/" .path }}/webhook/ | |
| pathType: {{ .pathType }} | |
| backend: | |
| service: | |
| name: {{ $fullName }}-webhook | |
| port: | |
| number: {{ $.Values.webhook.service.port | default 80 }} | |
| # Note: The default URL for manual workflow executions is /webhook-test/*. Make sure that these URLs route to your main process. | |
| - path: {{ . }}webhook-test/ | |
| pathType: Prefix | |
| - path: {{ trimSuffix "/" .path }}/webhook-test/ | |
| pathType: {{ .pathType }} | |
| backend: | |
| service: | |
| name: {{ $fullName }} | |
| port: | |
| number: {{ $.Values.main.service.port | default 80 }} | |
| - path: {{ . }}webhook-waiting/ | |
| pathType: Prefix | |
| number: {{ $.Values.main.service.port | default 80 }} | |
| - path: {{ trimSuffix "/" .path }}/webhook-waiting/ | |
| pathType: {{ .pathType }} | |
| backend: | |
| service: | |
| name: {{ $fullName }}-webhook | |
| port: | |
| number: {{ $.Values.webhook.service.port | default 80 }} | |
| - path: {{ trimSuffix "/" .path }}/form/ | |
| pathType: {{ .pathType }} | |
| backend: | |
| service: | |
| name: {{ $fullName }}-webhook | |
| port: | |
| number: {{ $.Values.webhook.service.port | default 80 }} | |
| - path: {{ . }}form/ | |
| pathType: Prefix | |
| - path: {{ trimSuffix "/" .path }}/form-waiting/ | |
| pathType: {{ .pathType }} | |
| backend: | |
| service: | |
| name: {{ $fullName }}-webhook | |
| port: | |
| number: {{ $.Values.webhook.service.port | default 80 }} | |
| - path: {{ trimSuffix "/" .path }}/form-test/ | |
| pathType: {{ .pathType }} | |
| backend: | |
| service: | |
| name: {{ $fullName }} | |
| port: | |
| number: {{ $.Values.main.service.port | default 80 }} | |
| {{- end }} | |
| {{- if $.Values.webhook.enabled }} | |
| - path: {{ trimSuffix "/" .path }}/webhook/ | |
| pathType: Prefix | |
| backend: | |
| service: | |
| name: {{ $fullName }}-webhook | |
| port: | |
| number: {{ $.Values.webhook.service.port | default 80 }} | |
| # Note: The default URL for manual workflow executions is /webhook-test/*. Make sure that these URLs route to your main process. | |
| - path: {{ trimSuffix "/" .path }}/webhook-test/ | |
| pathType: Prefix | |
| backend: | |
| service: | |
| name: {{ $fullName }} | |
| port: | |
| number: {{ $.Values.main.service.port | default 80 }} | |
| - path: {{ trimSuffix "/" .path }}/webhook-waiting/ | |
| pathType: Prefix | |
| backend: | |
| service: | |
| name: {{ $fullName }}-webhook | |
| port: | |
| number: {{ $.Values.webhook.service.port | default 80 }} | |
| - path: {{ trimSuffix "/" .path }}/form/ | |
| pathType: Prefix | |
| backend: | |
| service: | |
| name: {{ $fullName }}-webhook | |
| port: | |
| number: {{ $.Values.webhook.service.port | default 80 }} | |
| - path: {{ trimSuffix "/" .path }}/form-waiting/ | |
| pathType: Prefix | |
| backend: | |
| service: | |
| name: {{ $fullName }}-webhook | |
| port: | |
| number: {{ $.Values.webhook.service.port | default 80 }} | |
| - path: {{ trimSuffix "/" .path }}/form-test/ | |
| pathType: Prefix | |
| backend: | |
| service: | |
| name: {{ $fullName }} | |
| port: | |
| number: {{ $.Values.main.service.port | default 80 }} | |
| {{- end }} |
| ## N8N Specific Config Section in Values File | ||
|
|
||
| Every possible n8n config value can be set, | ||
| even if it is not mentioned in the excerpt below. | ||
| Treat the n8n provided configuration documentation as the source of truth, | ||
| this Charts just forwards everything down to the n8n pods. | ||
|
|
There was a problem hiding this comment.
Tighten wording + fix grammar (“n8n-provided”, “this chart”).
This section reads a bit unpolished and includes a grammar issue flagged by static analysis.
-Every possible n8n config value can be set,
+Every possible n8n config value can be set,
even if it is not mentioned in the excerpt below.
-Treat the n8n provided configuration documentation as the source of truth,
-this Charts just forwards everything down to the n8n pods.
+Treat the n8n-provided configuration documentation as the source of truth;
+this chart just forwards everything down to the n8n pods.📝 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.
| ## N8N Specific Config Section in Values File | |
| Every possible n8n config value can be set, | |
| even if it is not mentioned in the excerpt below. | |
| Treat the n8n provided configuration documentation as the source of truth, | |
| this Charts just forwards everything down to the n8n pods. | |
| ## N8N Specific Config Section in Values File | |
| Every possible n8n config value can be set, | |
| even if it is not mentioned in the excerpt below. | |
| Treat the n8n-provided configuration documentation as the source of truth; | |
| this chart just forwards everything down to the n8n pods. |
🧰 Tools
🪛 LanguageTool
[grammar] ~145-~145: Use a hyphen to join words.
Context: ...oned in the excerpt below. Treat the n8n provided configuration documentation as ...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
In README.md around lines 141-147, the wording is unpolished and has grammar
issues (use "n8n-provided" and "this chart"). Replace the current sentences with
a tighter, grammatically-correct version such as: "Every possible n8n
configuration value can be set; see the n8n-provided configuration documentation
as the source of truth — this chart forwards all settings to the n8n pods."
Ensure hyphenation on "n8n-provided" and use "this chart" (singular) along with
concise punctuation.
| # ... next k8s specific values section | ||
| ``` | ||
| ## Kubernetes Specific Values Section | ||
|
|
||
| this section of the `yaml` file contains the typical Kubernetes specific setting | ||
| related to the application deployment and operation but not the application itself. | ||
|
|
There was a problem hiding this comment.
Fix broken Markdown fencing/indentation around the “Kubernetes Specific Values Section”.
The closing fence appears indented as if it’s part of the YAML block (Line 181-182), which can break rendering and copy/paste. Also start the paragraph with a capital letter.
- # ... next k8s specific values section
- ```
-## Kubernetes Specific Values Section
+ # ... next k8s specific values section
+```
+
+## Kubernetes Specific Values Section
@@
-this section of the `yaml` file contains the typical Kubernetes specific setting
+This section of the `yaml` file contains typical Kubernetes-specific settings
related to the application deployment and operation but not the application itself.Also applies to: 181-182
🤖 Prompt for AI Agents
In README.md around lines 180 to 186, the Markdown code-fence and paragraph
capitalization are incorrect: move the closing triple-backtick out of the
indented YAML block so the fence is on its own line, ensure an opening fence
precedes the "## Kubernetes Specific Values Section" heading (so the fenced
block is properly closed), and change the paragraph start to "This" and reword
to "typical Kubernetes-specific settings" to fix capitalization and hyphenation;
update both occurrences noted (lines ~181-182 and the paragraph) accordingly.
This adds two new ingress paths: - form-waiting/ routes to the webhook service - form-test/ routes to the main service Signed-off-by: Yoni Golob <[email protected]>
yagni92
force-pushed
the
feat/add-form-paths-ingress
branch
from
eae5f20 to
5f775d4
Compare
3 participants
What this PR does / why we need it
This PR adds support for n8n's form functionality by adding two new ingress paths to the Helm chart:
form-waiting/routes to the webhook service for handling form submissions in waiting stateform-test/routes to the main service for form testingThese paths are essential for n8n's form to work properly.
Special notes for your reviewer
The routing logic follows the existing pattern in the ingress template:
form/andwebhook-waiting/paths)webhook-test/path)This change is backward compatible and only adds new functionality when the webhook service is enabled.
Checklist
Please place an 'x' in all applicable fields and remove unrelated items.
Version and Documentation
Chart.yamlfollowing semantic versioningChart.yamlif applicableartifacthub.io/changessection updated inChart.yaml(see ArtifactHub annotation reference)README.mdTesting and Validation
ah lintlocally without errorsct lint --chart-dirs charts/n8n --charts charts/n8n --validate-maintainers=false/examplesdirectorySummary by CodeRabbit
New Features
Version
✏️ Tip: You can customize this high-level summary in your review settings.