-
Notifications
You must be signed in to change notification settings - Fork 0
Add proper support for affinities, tolerations, selectors #216
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
Conversation
WalkthroughThe Helm chart's node-exporter DaemonSet template is refactored to externalize hard-coded affinity, nodeSelector, and tolerations configurations. These are now conditionally rendered from corresponding Helm values, enabling users to customize pod scheduling constraints without modifying the template. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
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 |
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: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
helm-chart/zxporter/templates/node-exporter-daemonset.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build Docker Image
- GitHub Check: Run make test
- GitHub Check: Build Docker Image
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
helm-chart/zxporter/templates/node-exporter-daemonset.yaml (2)
124-129: LGTM!The tolerations implementation correctly preserves the hard-coded
NoSchedule/Existstoleration (line 125-126) while allowing users to append additional tolerations. Since tolerations are list items, there's no risk of conflicts when merging user-provided values.
115-118: No breaking change—conditional affinity rendering is correct.The default values file provides
affinity: {}, so the conditional rendering at lines 115-118 works as intended. Users can override this by providing custom affinity rules via.Values.affinity.
| nodeSelector: | ||
| kubernetes.io/os: linux | ||
| {{- with .Values.nodeSelector }} | ||
| {{- toYaml . | nindent 8 }} | ||
| {{- end }} |
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.
Guard against duplicate nodeSelector keys.
The hard-coded kubernetes.io/os: linux selector at line 120 will conflict if users provide the same key in .Values.nodeSelector. While most YAML parsers will accept duplicate keys and use the last value, this creates confusion and is technically invalid YAML.
Consider one of these approaches:
Option 1: Remove the hard-coded selector and include it in default values
nodeSelector:
- kubernetes.io/os: linux
{{- with .Values.nodeSelector }}
{{- toYaml . | nindent 8 }}
{{- end }}Then ensure values.yaml contains:
nodeSelector:
kubernetes.io/os: linuxOption 2: Keep hard-coded and document the restriction
Add a comment above line 119:
+ # Note: kubernetes.io/os is hard-coded to "linux". Do not override in .Values.nodeSelector
nodeSelector:
kubernetes.io/os: linux🤖 Prompt for AI Agents
In helm-chart/zxporter/templates/node-exporter-daemonset.yaml around lines
119-123 there is a hard-coded nodeSelector key kubernetes.io/os: linux that may
duplicate a user-provided key in .Values.nodeSelector; either remove the
hard-coded entry and add kubernetes.io/os: linux to the chart's values.yaml
default nodeSelector, or keep the hard-coded entry and add a clear comment above
these lines documenting that users must not set kubernetes.io/os in
.Values.nodeSelector (and optionally validate/merge values in _helpers.tpl to
avoid duplicates).
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.