-
Notifications
You must be signed in to change notification settings - Fork 811
feat(helm): permits deployment strategy to helm chart #9558
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
base: master
Are you sure you want to change the base?
feat(helm): permits deployment strategy to helm chart #9558
Conversation
…t-Diot Signed-off-by: David Aparicio <david.aparicio@free.fr>
WalkthroughAdds three Helm values for Deployment strategy: 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. 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
🤖 Fix all issues with AI agents
In `@pkg/helm/templates/deployment.yaml`:
- Around line 13-21: The template accesses
.Values.strategy.rollingUpdate.maxSurge and
.Values.strategy.rollingUpdate.maxUnavailable unguarded which will nil-panic
when strategy.type is "RollingUpdate" but .Values.strategy.rollingUpdate is
missing; update the deployment template to either (a) wrap the rollingUpdate
block in an additional guard like {{- if .Values.strategy.rollingUpdate }} ...
{{- end }} so it only renders when rollingUpdate exists, or (b) use the Helm
default function to provide safe fallbacks (e.g. default values for
.Values.strategy.rollingUpdate.maxSurge and .maxUnavailable) and reference
.Values.strategy and strategy.type to keep behavior identical when set.
Signed-off-by: David Aparicio <david.aparicio@free.fr>
Test Results
spec:
replicas: 1
selector:
...→ Kubernetes uses default RollingUpdate with 25% values
spec:
replicas: 1
strategy:
type: RollingUpdate
selector:
...→ Kubernetes applies default rollingUpdate parameters (25%)
spec:
replicas: 1
strategy:
type: RollingUpdate
rollingUpdate:
maxSurge: 1
maxUnavailable: 0
selector:
...→ Uses custom values
spec:
replicas: 1
strategy:
type: Recreate
selector:
...→ No rollingUpdate section (as expected) |
thx to @Clement-Diot to reveal this issue (we need to use "recreate" strategy) :)
Summary
Add optional deployment strategy configuration to the Helm chart, enabling users to specify RollingUpdate or Recreate strategy types.
Motivation
The default Kubernetes RollingUpdate strategy causes deployment deadlocks with single-replica deployments using ReadWriteOnce PVCs (the chart's default configuration). During rolling updates, the new pod cannot mount the PVC because it's still attached to the old pod, creating a deadlock situation.
Changes
templates/deployment.yaml: Added conditional strategy block that renders only when strategy values are provided
README.md: Documented new strategy.* configuration options
Key Features
Example Usage
For single replica with ReadWriteOnce PVC (recommended)
For custom rolling updates
My Test Plan
helm template test-pgadmin . --dry-runhelm template test-pgadmin . --set strategy.type=RollingUpdate --set strategy.rollingUpdate.maxSurge=1 --set strategy.rollingUpdate.maxUnavailable=0helm template test-pgadmin . --set strategy.type=Recreate | grep -A 12 "kind: Deployment"Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.