Support revisionhistorylimit #221
Conversation
WalkthroughAdded Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
charts/n8n/values.yaml (1)
133-135: Deduplicate the identicalrevisionHistoryLimitdefaultsThe same literal value (10) is repeated for
main,worker, andwebhook. Consider a single source of truth to avoid accidental drift—e.g.# top-level default revisionHistoryLimit: 10and then in each section:
revisionHistoryLimit: *revisionHistoryLimit # YAML anchor/aliasor reference it in the templates with
default.
This keeps the chart DRY and reduces maintenance overhead.Also applies to: 326-328, 515-517
charts/n8n/templates/deployment.yaml (1)
15-19: Provide a fallback in case the value is unsetIf a user deletes the key or passes it as
null, Helm will render an empty value resulting in an invalid manifest (revisionHistoryLimit:). Add a default to keep the manifest valid:- revisionHistoryLimit: {{ .Values.main.revisionHistoryLimit }} + revisionHistoryLimit: {{ default 10 .Values.main.revisionHistoryLimit }}This mirrors Kubernetes’ own default (10) while remaining override-able.
charts/n8n/templates/deployment.worker.yaml (1)
18-20: Same fallback recommendation as for the main deployment- revisionHistoryLimit: {{ .Values.worker.revisionHistoryLimit }} + revisionHistoryLimit: {{ default 10 .Values.worker.revisionHistoryLimit }}charts/n8n/templates/deployment.webhook.yaml (1)
19-22: Same fallback recommendation as for the main deployment- revisionHistoryLimit: {{ .Values.webhook.revisionHistoryLimit }} + revisionHistoryLimit: {{ default 10 .Values.webhook.revisionHistoryLimit }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
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)
|
Thoughts on using YAML anchors to reduce repetition here? |
|
For a oneliner I think it adds more confusion than it helps but im fine with whatever maintainers decide |
|
Testing this today. Thank you for the contribution |
|
Would you also be able to bump the chart version (follow the Chart Versioning Schema). Then in |
DrFaust92
force-pushed
the
revisionhistorylimit
branch
from
e2b754b to
39aea42
Compare
|
RoseSecurity added changelog |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
charts/n8n/Chart.yaml (1)
37-38: Tighten ArtifactHub change entry and add context/link.Minor polish: make it explicit that all Deployments are covered and add a link back to the PR for traceability in ArtifactHub.
Apply this diff:
artifacthub.io/changes: | - - kind: added - description: "Support setting revisionHistoryLimit" + - kind: added + description: "Support setting revisionHistoryLimit on main, worker and webhook Deployments." + links: + - name: "PR #221" + url: "https://github.com/8gears/n8n-helm-chart/pull/221"charts/n8n/values.yaml (4)
133-135: Good default; consider clarifying behavior when set to 0.Defaulting to 10 mirrors Kubernetes’ default. Add a brief note that 0 disables revision history to help users tune cleanup.
Apply this diff:
# How many old ReplicaSets for the Deployment to retain to allow rollback. + # Note: set to 0 to disable retention entirely. revisionHistoryLimit: 10
326-328: Mirror the same doc hint for worker.Keep docs consistent across components.
Apply this diff:
# How many old ReplicaSets for the Deployment to retain to allow rollback. + # Note: set to 0 to disable retention entirely. revisionHistoryLimit: 10
515-517: Mirror the same doc hint for webhook.Keep docs consistent across components.
Apply this diff:
# How many old ReplicaSets for the Deployment to retain to allow rollback. + # Note: set to 0 to disable retention entirely. revisionHistoryLimit: 10
133-135: Optional: ConsolidaterevisionHistoryLimitDefaultsThe three identical defaults can be centralized to reduce duplication and simplify future changes:
• In
charts/n8n/values.yamlat lines 134, 327, 516, you definerevisionHistoryLimit: 10under each of
main,worker, andwebhook.
• In the templates—
charts/n8n/templates/deployment.yaml(line 18)charts/n8n/templates/deployment.worker.yaml(line 19)charts/n8n/templates/deployment.webhook.yaml(line 21)
—eachspec.revisionHistoryLimitreferences its corresponding section:revisionHistoryLimit: {{ .Values.<component>.revisionHistoryLimit }}Proposed approach:
- Add a top-level common default in
values.yaml:common: revisionHistoryLimit: 10- Remove the duplicated
revisionHistoryLimit: 10entries undermain,worker, andwebhook(or leave them empty for overrides).- Update each template to fall back to the shared value:
(and similarly forspec: revisionHistoryLimit: {{ .Values.main.revisionHistoryLimit | default .Values.common.revisionHistoryLimit }}workerandwebhook).This keeps a single source of truth while still allowing per-component overrides.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
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 (3)
- charts/n8n/templates/deployment.yaml
- charts/n8n/templates/deployment.webhook.yaml
- charts/n8n/templates/deployment.worker.yaml
🔇 Additional comments (1)
charts/n8n/Chart.yaml (1)
3-3: SemVer bump to 1.1.0 is appropriate for an additive feature.Feature addition without breaking changes warrants a minor version bump. Looks good.
|
RoseSecurity gentle reminder. |
|
If you change the value.yaml please update the Readme accordingly and in the right spot |
Signed-off-by: drfaust92 <[email protected]>
Signed-off-by: drfaust92 <[email protected]>
Signed-off-by: drfaust92 <[email protected]>
Signed-off-by: drfaust92 <[email protected]>
DrFaust92
force-pushed
the
revisionhistorylimit
branch
from
084ee6b to
e6924d3
Compare
|
Vad1mo I can do it, seems a bit too manual to me though. I would recommend a more automated method using https://github.com/norwoodj/helm-docs to generate it from values.yaml. wdyt? |
Signed-off-by: drfaust92 <[email protected]>
|
Vad1mo added refs in docs in any case |
I don't like the table output, it kills readability and take a huge amount of space |
3 participants
Summary by CodeRabbit