-
Notifications
You must be signed in to change notification settings - Fork 203
add extraContainers support #252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds a new Helm values key Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Helm as Helm template engine
participant Kube as Kubernetes API
User->>Helm: helm install/upgrade (values include extraContainers)
Helm->>Helm: For each of main/worker/webhook: check .Values.<scope>.extraContainers
alt extraContainers provided
Helm->>Helm: tpl(toYaml(extraContainers)) → render additional container blocks
note right of Helm #E6F7FF: Inject into\n.spec.template.spec.containers
else not provided
Helm->>Helm: Render default containers only
end
Helm->>Kube: Apply rendered Deployment manifests
Kube-->>User: Deployments created/updated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
f867d60
to
086f9aa
Compare
086f9aa
to
04bedfa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (8)
charts/n8n/templates/deployment.worker.yaml (1)
118-120
: Worker sidecars block is correct and consistentGood use of tpl(toYaml(...)) with proper indentation.
Optional: allow merging in a shared .Values.extraContainers to reduce duplication when the same sidecars are needed on all pods.
charts/n8n/templates/deployment.webhook.yaml (1)
122-124
: Webhook sidecars block is correctMatches the pattern used for main/worker and will append valid container specs.
Optional: consider supporting a shared .Values.extraContainers for common sidecars.
README.md (3)
358-364
: Clarify what’s allowed in extraContainers entriesSuggest adding a note that each item is a full Kubernetes Container spec and Helm templating is supported (tpl), plus a brief example including resources and a probe.
550-556
: Worker docs: include a sidecar example tailored to workersAdd an example showing envFrom/secretKeyRef and ports typical for worker‑side runners, as mentioned in the PR description.
742-748
: Webhook docs: add a minimal HTTP sidecar exampleConsider adding a webhook‑specific sidecar example (e.g., tiny HTTP helper) and remind users to set any needed volumeMounts within the sidecar.
charts/n8n/values.yaml (3)
267-273
: Document that items are full Container specs and can use templatingAdd a one‑liner: entries accept the standard K8s Container fields; Helm templating is supported; include a short example with resources.
459-465
: Worker: add example with envFrom/portsProvide a worker‑focused example (envFrom with secretKeyRef, ports, readinessProbe) to guide typical runner sidecars.
650-656
: Webhook: add example and note about volume mountsInclude a brief example and a note that sidecars can declare their own volumeMounts which must match pod volumes defined under extraVolumes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md
(3 hunks)charts/n8n/Chart.yaml
(2 hunks)charts/n8n/templates/deployment.webhook.yaml
(1 hunks)charts/n8n/templates/deployment.worker.yaml
(1 hunks)charts/n8n/templates/deployment.yaml
(1 hunks)charts/n8n/values.yaml
(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T08:05:39.372Z
Learnt from: tsch157
PR: 8gears/n8n-helm-chart#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
🔇 Additional comments (3)
charts/n8n/Chart.yaml (2)
3-4
: Verify appVersion is a stable releaseVersion bumps look fine. Please double‑check n8n 1.114.3 isn’t a pre‑release before publishing.
Based on learnings
37-38
: Changelog entry LGTMAccurate “added” entry for extraContainers support.
charts/n8n/templates/deployment.yaml (1)
103-105
: Sidecar rendering under containers looks correcttpl(toYaml .Values.main.extraContainers) | nindent 8 correctly appends sidecars to the containers list.
Optional: consider a shared .Values.extraContainers to DRY identical sidecars across main/worker/webhook.
Please verify the rendered output in your local Helm environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
charts/n8n/templates/deployment.worker.yaml (1)
118-120
: Placement/indentation LGTM; consider merging global + worker extras.Rendering with tpl(toYaml ...) at nindent 8 correctly appends sidecars under containers:. To align with the PR’s “top‑level extraContainers” intent and reduce duplication, merge .Values.extraContainers with .Values.worker.extraContainers before rendering.
Apply this diff to support both sources:
- {{- if .Values.worker.extraContainers }} - {{ tpl (toYaml .Values.worker.extraContainers) . | nindent 8 }} - {{- end }} + {{- $extra := list }} + {{- if .Values.extraContainers }}{{- $extra = concat $extra .Values.extraContainers }}{{- end }} + {{- if .Values.worker.extraContainers }}{{- $extra = concat $extra .Values.worker.extraContainers }}{{- end }} + {{- if $extra }} + {{ tpl (toYaml $extra) . | nindent 8 }} + {{- end }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md
(3 hunks)charts/n8n/Chart.yaml
(2 hunks)charts/n8n/templates/deployment.webhook.yaml
(1 hunks)charts/n8n/templates/deployment.worker.yaml
(1 hunks)charts/n8n/templates/deployment.yaml
(1 hunks)charts/n8n/values.yaml
(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- README.md
- charts/n8n/Chart.yaml
- charts/n8n/values.yaml
- charts/n8n/templates/deployment.webhook.yaml
- charts/n8n/templates/deployment.yaml
What this PR does / why we need it
This PR adds basic support for specifying
extraContainers
(sidecar) which can be used in main, worker, and webhook deployments.I want this feature added so I can add external n8n runner sidecar with the workers but it is generic enough to allow users to setup any sidecar they need. I tested this locally using:
Special notes for your reviewer
This PR was originally submitted by ArnaudTA in PR #180 but it has sat a month without addressing reviewer concerns.
Checklist
Version and Documentation
Chart.yaml
following semantic versioningChart.yaml
if applicableartifacthub.io/changes
section updated inChart.yaml
(see ArtifactHub annotation reference)README.md
Testing and Validation
ah lint
locally without errorsct lint --chart-dirs charts/n8n --charts charts/n8n --validate-maintainers=false
/examples
directorySummary by CodeRabbit
New Features
Documentation
Chores