feat: add lifecycle and terminationGracePeriodSeconds values#110
Conversation
Lets a preStop hook keep the pod serving while a load balancer finishes deregistering its target during a rolling update, avoiding 502s on the scale-down side. Both default to no-ops, so behavior is unchanged unless set. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 48 minutes and 19 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR extends the sourcebot Helm chart to support Kubernetes pod lifecycle hooks and graceful termination configuration. It adds two new optional values: ChangesKubernetes Graceful Termination Configuration
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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/sourcebot/values.schema.json (1)
173-175: ⚡ Quick winConsider adding a minimum constraint to prevent negative values.
Kubernetes
terminationGracePeriodSecondsonly accepts 0 or positive integers. Adding"minimum": 0would catch invalid negative values at schema validation time.✨ Proposed schema enhancement
"terminationGracePeriodSeconds": { - "type": ["integer", "null"] + "type": ["integer", "null"], + "minimum": 0 },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/sourcebot/values.schema.json` around lines 173 - 175, The schema for terminationGracePeriodSeconds currently allows integers but not constrained against negatives; update the values.schema.json entry for "terminationGracePeriodSeconds" to include a "minimum": 0 (while keeping the existing "type": ["integer","null"]) so schema validation rejects negative values; locate the "terminationGracePeriodSeconds" property and add the "minimum": 0 constraint.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/sourcebot/templates/deployment.yaml`:
- Around line 33-35: The template guard using "{{- with
$.Values.sourcebot.terminationGracePeriodSeconds }}" drops valid 0 values;
change the guard to test for non-nil explicitly by replacing the with block with
an if that checks "ne $.Values.sourcebot.terminationGracePeriodSeconds nil" so
terminationGracePeriodSeconds: {{ . }} (or use the value expression) is rendered
even when set to 0; update the matching "{{- end }}" accordingly and keep the
same symbol $.Values.sourcebot.terminationGracePeriodSeconds in the template.
---
Nitpick comments:
In `@charts/sourcebot/values.schema.json`:
- Around line 173-175: The schema for terminationGracePeriodSeconds currently
allows integers but not constrained against negatives; update the
values.schema.json entry for "terminationGracePeriodSeconds" to include a
"minimum": 0 (while keeping the existing "type": ["integer","null"]) so schema
validation rejects negative values; locate the "terminationGracePeriodSeconds"
property and add the "minimum": 0 constraint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8deb8f77-cc72-4ce2-83d3-22c104c2bcd4
📒 Files selected for processing (4)
CHANGELOG.mdcharts/sourcebot/templates/deployment.yamlcharts/sourcebot/values.schema.jsoncharts/sourcebot/values.yaml
1 participant
What
Adds two passthrough values to the Sourcebot deployment:
sourcebot.lifecycle— container lifecycle hooks (e.g. apreStopsleep)sourcebot.terminationGracePeriodSeconds— pod termination grace periodBoth default to no-ops (
{}/null), so existing deployments are unchanged.Why
On EKS (and other ALB/NLB setups), rolling updates can return brief 502s on the scale-down side: when the old pod terminates, the load balancer may still route to it for a moment while it deregisters the target. A
preStopsleep keeps the pod serving during that deregistration window, andterminationGracePeriodSecondsensures the pod isn't SIGKILLed before the sleep + graceful shutdown complete.Example:
Changes
templates/deployment.yaml— renderlifecycle(container) andterminationGracePeriodSeconds(pod spec), guarded bywithso they're omitted when unsetvalues.yaml— defaults + documentationvalues.schema.json— register both (schema isadditionalProperties: false)CHANGELOG.md—[Unreleased]entryTesting
helm templaterenders both fields correctly when set and omits them at defaults; schema validation passes.🤖 Generated with Claude Code
Summary by CodeRabbit