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>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds PodDisruptionBudget support to the LFX v2 auth service Helm chart: a new conditional template renders a policy/v1 PodDisruptionBudget when 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-auth-service Helm chart to control voluntary disruptions for the auth-service pods.
Changes:
- Introduces
podDisruptionBudgetconfiguration invalues.yaml(disabled by default). - Adds a new
PodDisruptionBudgetmanifest template supportingminAvailable/maxUnavailable.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
charts/lfx-v2-auth-service/values.yaml |
Adds values for enabling/configuring a PDB. |
charts/lfx-v2-auth-service/templates/pdb.yaml |
New PDB template gated by podDisruptionBudget.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: {{ . }} |
There was a problem hiding this comment.
PodDisruptionBudget requires exactly one of spec.minAvailable or spec.maxUnavailable. As written, enabling the PDB without setting either value will render an invalid manifest, and setting both will also be rejected by the Kubernetes API. Consider adding a Helm validation (e.g., fail) to enforce “one of” semantics, and render the field(s) based on key presence rather than with so explicit 0 values don’t get dropped.
| replicaCount: 3 | ||
|
|
||
| podDisruptionBudget: | ||
| enabled: false |
There was a problem hiding this comment.
The values comments don’t indicate that exactly one of minAvailable or maxUnavailable must be set when podDisruptionBudget.enabled=true. Adding a short note here would help prevent users from enabling the feature and getting an invalid PDB rendered.
| enabled: false | |
| enabled: false | |
| # Exactly one of minAvailable or maxUnavailable must be set when enabled is true. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
charts/lfx-v2-auth-service/values.yaml (1)
7-10: DocumentminAvailablevsmaxUnavailableas mutually exclusive.A short inline note here would prevent invalid PDB configurations from being set in user overrides.
Suggested values comment tweak
podDisruptionBudget: enabled: false + # Set exactly one of the following fields: # minAvailable: 1 # maxUnavailable: 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/lfx-v2-auth-service/values.yaml` around lines 7 - 10, Add a short inline comment under podDisruptionBudget in values.yaml documenting that minAvailable and maxUnavailable are mutually exclusive and only one should be set in overrides (include guidance to prefer one over the other or leave both commented to use the chart default), referencing the exact keys minAvailable and maxUnavailable so users cannot accidentally provide both; keep the note concise and colocated with the existing commented example lines.
🤖 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-auth-service/templates/pdb.yaml`:
- Around line 14-19: Replace the current `with` blocks with presence checks that
treat 0 as a valid value and ensure mutual exclusivity: use `hasKey` (or
equivalent presence test) against `.Values.podDisruptionBudget` to render
`minAvailable` only if `hasKey .Values.podDisruptionBudget "minAvailable"` and
otherwise render `maxUnavailable` only if `hasKey .Values.podDisruptionBudget
"maxUnavailable"`, i.e. implement an if / else-if around those keys so you never
render both fields and zero values are emitted correctly.
---
Nitpick comments:
In `@charts/lfx-v2-auth-service/values.yaml`:
- Around line 7-10: Add a short inline comment under podDisruptionBudget in
values.yaml documenting that minAvailable and maxUnavailable are mutually
exclusive and only one should be set in overrides (include guidance to prefer
one over the other or leave both commented to use the chart default),
referencing the exact keys minAvailable and maxUnavailable so users cannot
accidentally provide both; keep the note concise and colocated with the existing
commented example lines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb43b6ea-a9e0-44a2-9534-796eaef43585
📒 Files selected for processing (2)
charts/lfx-v2-auth-service/templates/pdb.yamlcharts/lfx-v2-auth-service/values.yaml
| {{- with .Values.podDisruptionBudget.minAvailable }} | ||
| minAvailable: {{ . }} | ||
| {{- end }} | ||
| {{- with .Values.podDisruptionBudget.maxUnavailable }} | ||
| maxUnavailable: {{ . }} | ||
| {{- end }} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Kubernetes policy/v1 PodDisruptionBudget, are minAvailableandmaxUnavailable mutually exclusive, and is at least one of them required?
💡 Result:
Yes—minAvailable and maxUnavailable are mutually exclusive in a policy/v1 PodDisruptionBudget; you can set only one of them. [1][2]
Is at least one required?
They are not strictly required by the API schema (both fields are optional), but a PDB is only meaningful if you set one or the other; older kubectl behavior even defaulted minAvailable: 1 when neither was specified (Kubernetes v1.8 and earlier). [1][3]
Sources
[1] Kubernetes docs – Specifying a Disruption Budget for your Application (PDB has fields; only one of minAvailable/maxUnavailable; historical default note)
[2] Kubernetes API reference – PodDisruptionBudgetSpec.maxUnavailable (“mutually exclusive with minAvailable”)
[3] Older Kubernetes task doc mirror noting kubectl defaulting minAvailable when neither field set (v1.8 and earlier)
🏁 Script executed:
cd /tmp && find . -name "pdb.yaml" -type f 2>/dev/null | head -20Repository: linuxfoundation/lfx-v2-auth-service
Length of output: 61
🏁 Script executed:
find . -path "*lfx-v2-auth-service*" -name "pdb.yaml" -type f 2>/dev/nullRepository: linuxfoundation/lfx-v2-auth-service
Length of output: 126
🏁 Script executed:
git ls-files | grep -i "pdb.yaml"Repository: linuxfoundation/lfx-v2-auth-service
Length of output: 124
🏁 Script executed:
cat -n charts/lfx-v2-auth-service/templates/pdb.yamlRepository: linuxfoundation/lfx-v2-auth-service
Length of output: 746
Guard PDB field rendering to avoid invalid specs.
Lines 14–19 use with, which treats 0 as empty and won't render. The current logic can emit both minAvailable and maxUnavailable fields (or neither) when enabled, which violates Kubernetes' policy/v1 PodDisruptionBudget spec that requires these fields to be mutually exclusive and at least one to be present.
Proposed Helm guard + rendering fix
{{- if .Values.podDisruptionBudget.enabled }}
+{{- $pdb := .Values.podDisruptionBudget }}
+{{- $hasMin := hasKey $pdb "minAvailable" }}
+{{- $hasMax := hasKey $pdb "maxUnavailable" }}
+{{- if and $hasMin $hasMax }}
+{{- fail "Set only one of podDisruptionBudget.minAvailable or podDisruptionBudget.maxUnavailable" }}
+{{- end }}
+{{- if not (or $hasMin $hasMax) }}
+{{- fail "Set one of podDisruptionBudget.minAvailable or podDisruptionBudget.maxUnavailable when podDisruptionBudget.enabled=true" }}
+{{- end }}
---
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 }}
+ {{- if $hasMin }}
+ minAvailable: {{ $pdb.minAvailable }}
+ {{- end }}
+ {{- if $hasMax }}
+ maxUnavailable: {{ $pdb.maxUnavailable }}
+ {{- end }}
{{- end }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{- with .Values.podDisruptionBudget.minAvailable }} | |
| minAvailable: {{ . }} | |
| {{- end }} | |
| {{- with .Values.podDisruptionBudget.maxUnavailable }} | |
| maxUnavailable: {{ . }} | |
| {{- end }} | |
| {{- if .Values.podDisruptionBudget.enabled }} | |
| {{- $pdb := .Values.podDisruptionBudget }} | |
| {{- $hasMin := hasKey $pdb "minAvailable" }} | |
| {{- $hasMax := hasKey $pdb "maxUnavailable" }} | |
| {{- if and $hasMin $hasMax }} | |
| {{- fail "Set only one of podDisruptionBudget.minAvailable or podDisruptionBudget.maxUnavailable" }} | |
| {{- end }} | |
| {{- if not (or $hasMin $hasMax) }} | |
| {{- fail "Set one of podDisruptionBudget.minAvailable or podDisruptionBudget.maxUnavailable when podDisruptionBudget.enabled=true" }} | |
| {{- end }} | |
| --- | |
| apiVersion: policy/v1 | |
| kind: PodDisruptionBudget | |
| metadata: | |
| name: {{ .Chart.Name }} | |
| namespace: {{ .Release.Namespace }} | |
| spec: | |
| selector: | |
| matchLabels: | |
| app: {{ .Chart.Name }} | |
| {{- if $hasMin }} | |
| minAvailable: {{ $pdb.minAvailable }} | |
| {{- end }} | |
| {{- if $hasMax }} | |
| maxUnavailable: {{ $pdb.maxUnavailable }} | |
| {{- end }} | |
| {{- end }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/lfx-v2-auth-service/templates/pdb.yaml` around lines 14 - 19, Replace
the current `with` blocks with presence checks that treat 0 as a valid value and
ensure mutual exclusivity: use `hasKey` (or equivalent presence test) against
`.Values.podDisruptionBudget` to render `minAvailable` only if `hasKey
.Values.podDisruptionBudget "minAvailable"` and otherwise render
`maxUnavailable` only if `hasKey .Values.podDisruptionBudget "maxUnavailable"`,
i.e. implement an if / else-if around those keys so you never render both fields
and zero values are emitted correctly.
🤖 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>
Summary
podDisruptionBudgetvalues to Helm chart (disabled by default)minAvailableandmaxUnavailable🤖 Generated with Claude Code