Add support for runtimeClassName #194
Conversation
…inir-un-runtimeclass Add runtime class support
WalkthroughThe changes introduce a new optional configuration key, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Helm Chart
participant Kubernetes API
User->>Helm Chart: Set runtimeClassName in values.yaml (optional)
Helm Chart->>Kubernetes API: Deploy main/worker/webhook pod with runtimeClassName (if set)
Kubernetes API-->>Helm Chart: Pod scheduled with specified runtime class
Helm Chart-->>User: Deployment reflects runtimeClassName configuration
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 3
🧹 Nitpick comments (3)
README.md (3)
358-361: DocumentruntimeClassNamefor main pods
The newruntimeClassNamekey is introduced without context. Please add a descriptive comment explaining its purpose, default behavior, and an example usage to improve clarity.autoscaling: enabled: false - runtimeClassName: "" + # Kubernetes RuntimeClassName for the main n8n pod (e.g., "gvisor"); leave empty to use the default runtime + runtimeClassName: "" nodeSelector: {}
544-548: DocumentruntimeClassNamefor worker pods
Apply the same descriptive enhancement in the worker section so users understand how to use this setting and what the default means.autoscaling: enabled: false - runtimeClassName: "" + # Kubernetes RuntimeClassName for worker pods (e.g., "kata"); leave empty to use the cluster default runtime + runtimeClassName: "" nodeSelector: {}
730-734: DocumentruntimeClassNamefor webhook pods
The webhook section should also include an explanatory comment forruntimeClassName, detailing its intent, default, and an example value.autoscaling: enabled: false - runtimeClassName: "" + # Kubernetes RuntimeClassName for webhook pods (e.g., "gvisor"); leave empty for the default node runtime + runtimeClassName: "" nodeSelector: {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(3 hunks)charts/n8n/Chart.yaml(1 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 comments (4)
charts/n8n/Chart.yaml (1)
3-3: Approve chart version bump to v1.0.8
Incrementing the chart version to 1.0.8 is appropriate to bundle the newruntimeClassNamefeature. No further changes needed in metadata.charts/n8n/templates/deployment.yaml (1)
48-50:⚠️ Potential issueFix conditional inclusion of runtimeClassName for main pod
Usingwith .Values.main.runtimeClassNametreats empty strings as non-nil, so the block always renders, injectingruntimeClassName: "". Replace with anifcheck against an empty value to only include the field when set.Proposed diff:
- {{- with .Values.main.runtimeClassName }} - runtimeClassName: {{ . }} - {{- end }} + {{- if and .Values.main.runtimeClassName (ne .Values.main.runtimeClassName "") }} + runtimeClassName: {{ .Values.main.runtimeClassName }} + {{- end }}Likely an incorrect or invalid review comment.
charts/n8n/templates/deployment.webhook.yaml (1)
51-53:⚠️ Potential issueFix conditional inclusion of runtimeClassName for webhook pod
Thewithblock will run even if.Values.webhook.runtimeClassNameis an empty string, causing an unwanted empty field. Use anifcondition to guard against empty values.Proposed diff:
- {{- with .Values.webhook.runtimeClassName }} - runtimeClassName: {{ . }} - {{- end }} + {{- if and .Values.webhook.runtimeClassName (ne .Values.webhook.runtimeClassName "") }} + runtimeClassName: {{ .Values.webhook.runtimeClassName }} + {{- end }}Likely an incorrect or invalid review comment.
charts/n8n/templates/deployment.worker.yaml (1)
49-51:⚠️ Potential issueFix conditional inclusion of runtimeClassName for worker pod
An empty string is truthy in awithblock, so the template injectsruntimeClassName: ""inadvertently. Switch to anifcheck that excludes empty values.Proposed diff:
- {{- with .Values.worker.runtimeClassName }} - runtimeClassName: {{ . }} - {{- end }} + {{- if and .Values.worker.runtimeClassName (ne .Values.worker.runtimeClassName "") }} + runtimeClassName: {{ .Values.worker.runtimeClassName }} + {{- end }}Likely an incorrect or invalid review comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ng in values.yaml
|
@obeone, looks good. There are some errors, can, you take a look? |
|
@obeone can yoou fix the failing checks? |
|
The commit that appears to be failing is |
3 participants
Adding the ability to set a runtimeClassName
Summary by CodeRabbit