Conversation
🤖 Generated with [Claude Code](https://claude.com/claude-code) Issue: LFXV2-1166 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Trevor Bramwell <tbramwell@linuxfoundation.org>
WalkthroughA new Helm template for Kubernetes PodDisruptionBudget resources is added to the lfx-v2-committee-service chart. The template includes validation to prevent simultaneous configuration of both minAvailable and maxUnavailable parameters. Configuration values are introduced with the feature disabled by default, allowing operators to enable and configure disruption budgets as needed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds optional PodDisruptionBudget (PDB) support to the lfx-v2-committee-service Helm chart so operators can configure disruption limits via chart values.
Changes:
- Introduces
podDisruptionBudgetconfiguration invalues.yaml(disabled by default). - Adds a
PodDisruptionBudgetmanifest template supportingminAvailableandmaxUnavailable.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
charts/lfx-v2-committee-service/values.yaml |
Adds new podDisruptionBudget values block for configuring PDB behavior. |
charts/lfx-v2-committee-service/templates/pdb.yaml |
Adds a conditional PDB template rendered when the feature is enabled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {{- with .Values.podDisruptionBudget.minAvailable }} | ||
| minAvailable: {{ . }} | ||
| {{- end }} | ||
| {{- with .Values.podDisruptionBudget.maxUnavailable }} | ||
| maxUnavailable: {{ . }} | ||
| {{- end }} |
There was a problem hiding this comment.
The {{- with ... }} blocks under spec: trim the preceding newline, which can concatenate app: {{ .Chart.Name }} with minAvailable/maxUnavailable and render invalid YAML. Use non-trimming actions here (e.g., {{ with ... }} / {{ end }}) or otherwise ensure a newline is preserved before emitting these fields.
| {{- with .Values.podDisruptionBudget.minAvailable }} | |
| minAvailable: {{ . }} | |
| {{- end }} | |
| {{- with .Values.podDisruptionBudget.maxUnavailable }} | |
| maxUnavailable: {{ . }} | |
| {{- end }} | |
| {{ with .Values.podDisruptionBudget.minAvailable }} | |
| minAvailable: {{ . }} | |
| {{ end }} | |
| {{ with .Values.podDisruptionBudget.maxUnavailable }} | |
| maxUnavailable: {{ . }} | |
| {{ end }} |
| {{- if .Values.podDisruptionBudget.enabled }} | ||
| --- | ||
| apiVersion: policy/v1 | ||
| kind: PodDisruptionBudget | ||
| metadata: | ||
| name: {{ .Chart.Name }} | ||
| namespace: {{ .Release.Namespace }} | ||
| spec: | ||
| selector: | ||
| matchLabels: | ||
| app: {{ .Chart.Name }} | ||
| {{- with .Values.podDisruptionBudget.minAvailable }} | ||
| minAvailable: {{ . }} | ||
| {{- end }} | ||
| {{- with .Values.podDisruptionBudget.maxUnavailable }} | ||
| maxUnavailable: {{ . }} | ||
| {{- end }} |
There was a problem hiding this comment.
When podDisruptionBudget.enabled is true, this template can render an invalid PDB if neither minAvailable nor maxUnavailable is set, and it can also render an invalid PDB if both are set. Add Helm-time validation (e.g., fail/required) to enforce that exactly one of these values is provided when enabled.
| # minAvailable: 1 | ||
| # maxUnavailable: 1 |
There was a problem hiding this comment.
The values docs currently suggest both minAvailable and maxUnavailable can be configured, but PodDisruptionBudget requires exactly one of them. Consider updating these comments to indicate they are mutually exclusive and that one must be set when podDisruptionBudget.enabled=true.
| # minAvailable: 1 | |
| # maxUnavailable: 1 | |
| # minAvailable: 1 # Specify exactly one of minAvailable or maxUnavailable when enabled=true | |
| # maxUnavailable: 1 # Mutually exclusive with minAvailable; uncomment only one of these fields |
🤖 Generated with [Claude Code](https://claude.com/claude-code) Issue: LFXV2-1166 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Trevor Bramwell <tbramwell@linuxfoundation.org>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Issue: LFXV2-1166 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Trevor Bramwell <tbramwell@linuxfoundation.org>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/lfx-v2-committee-service/templates/pdb.yaml`:
- Around line 17-22: The PDB template can render an invalid empty spec when
.Values.podDisruptionBudget.enabled is true but neither
.Values.podDisruptionBudget.minAvailable nor
.Values.podDisruptionBudget.maxUnavailable is set; fix it by adding a
guard/validation that checks hasKey .Values.podDisruptionBudget "minAvailable"
or hasKey .Values.podDisruptionBudget "maxUnavailable" (or both) before
rendering the spec, and if both are missing call fail with a clear message
(e.g., using Helm's fail function) so the chart errors out instead of producing
an invalid PDB; update the conditional logic around the spec generation that
currently uses hasKey checks for minAvailable/maxUnavailable to enforce this
combined requirement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e573b12-f6b3-47b5-8e4a-3263713425f2
📒 Files selected for processing (2)
charts/lfx-v2-committee-service/templates/pdb.yamlcharts/lfx-v2-committee-service/values.yaml
| {{- if hasKey .Values.podDisruptionBudget "minAvailable" }} | ||
| minAvailable: {{ .Values.podDisruptionBudget.minAvailable }} | ||
| {{- end }} | ||
| {{- if hasKey .Values.podDisruptionBudget "maxUnavailable" }} | ||
| maxUnavailable: {{ .Values.podDisruptionBudget.maxUnavailable }} | ||
| {{- end }} |
There was a problem hiding this comment.
Missing validation: PDB requires at least one of minAvailable or maxUnavailable.
When podDisruptionBudget.enabled is true but neither minAvailable nor maxUnavailable is set, this template renders a PDB without a disruption budget spec, which is invalid. Kubernetes requires at least one of these fields.
🐛 Proposed fix to add validation
Add this validation after line 6:
{{- if and (hasKey .Values.podDisruptionBudget "minAvailable") (hasKey .Values.podDisruptionBudget "maxUnavailable") }}
{{- fail "podDisruptionBudget: cannot set both minAvailable and maxUnavailable" }}
{{- end }}
+{{- if not (or (hasKey .Values.podDisruptionBudget "minAvailable") (hasKey .Values.podDisruptionBudget "maxUnavailable")) }}
+ {{- fail "podDisruptionBudget: must set either minAvailable or maxUnavailable when enabled" }}
+{{- end }}
---🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/lfx-v2-committee-service/templates/pdb.yaml` around lines 17 - 22, The
PDB template can render an invalid empty spec when
.Values.podDisruptionBudget.enabled is true but neither
.Values.podDisruptionBudget.minAvailable nor
.Values.podDisruptionBudget.maxUnavailable is set; fix it by adding a
guard/validation that checks hasKey .Values.podDisruptionBudget "minAvailable"
or hasKey .Values.podDisruptionBudget "maxUnavailable" (or both) before
rendering the spec, and if both are missing call fail with a clear message
(e.g., using Helm's fail function) so the chart errors out instead of producing
an invalid PDB; update the conditional logic around the spec generation that
currently uses hasKey checks for minAvailable/maxUnavailable to enforce this
combined requirement.
Summary
podDisruptionBudgetvalues to Helm chart (disabled by default)minAvailableandmaxUnavailable🤖 Generated with Claude Code