fix(pvc): prevent PV rebinding on upgrade and fix missing annotations block#102
fix(pvc): prevent PV rebinding on upgrade and fix missing annotations block#102fatmcgav wants to merge 2 commits intosourcebot-dev:mainfrom
Conversation
… block Add `helm.sh/resource-policy: keep` unconditionally so the PVC survives helm uninstall, fix the annotations block which was previously absent when no custom annotations were set, and use `lookup` to pin `volumeName` on upgrades to prevent the PVC from being re-bound to a different PV. Also add some test coverage. Fixes sourcebot-dev#74
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 52 minutes and 46 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe PVC template now includes a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
charts/sourcebot/templates/pvc.yaml (1)
24-34:⚠️ Potential issue | 🔴 CriticalCritical:
volumeNamepinning is dead code for default installations.The
volumeNamelookup block (lines 30–32) is nested inside the{{- if .Values.sourcebot.persistence.storageClass }}guard opened on line 24 and closed on line 33. Sincesourcebot.persistence.storageClassdefaults to""(empty string, falsy — seecharts/sourcebot/values.yaml), the lookup will be skipped on the default path and the PVC will render withoutvolumeName, defeating the primary objective of this PR (issue#74). It only takes effect when a user explicitly sets a storage class.Move the
volumeNameblock outside the storageClass conditional.🛠️ Proposed fix
{{- if .Values.sourcebot.persistence.storageClass }} {{- if (eq "-" .Values.sourcebot.persistence.storageClass) }} storageClassName: "" {{- else }} storageClassName: {{ .Values.sourcebot.persistence.storageClass }} {{- end }} - {{- if (lookup "v1" "PersistentVolumeClaim" $.Release.Namespace $volumeName) }} - volumeName: {{ (lookup "v1" "PersistentVolumeClaim" $.Release.Namespace $volumeName).spec.volumeName }} - {{- end }} {{- end }} + {{- $existingPvc := lookup "v1" "PersistentVolumeClaim" $.Release.Namespace $volumeName }} + {{- if and $existingPvc $existingPvc.spec.volumeName }} + volumeName: {{ $existingPvc.spec.volumeName }} + {{- end }}Notes incorporated in the diff:
- Caches the
lookupresult instead of calling it twice on consecutive lines.- Guards on
$existingPvc.spec.volumeNamebeing non-empty, otherwise a pending/unbound PVC would rendervolumeName:with an empty value and break the spec.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/sourcebot/templates/pvc.yaml` around lines 24 - 34, The volumeName lookup block is incorrectly nested inside the storageClass guard so it never runs when .Values.sourcebot.persistence.storageClass is empty; move the lookup/volumeName logic out of the {{- if .Values.sourcebot.persistence.storageClass }}...{{- end }} block so it always executes, use lookup("v1","PersistentVolumeClaim",$ .Release.Namespace,$volumeName) once and store it in a variable (e.g. $existingPvc) instead of calling lookup twice, and only render volumeName: {{ $existingPvc.spec.volumeName }} when $existingPvc and $existingPvc.spec.volumeName are non-empty to avoid emitting an empty volumeName field.
🧹 Nitpick comments (1)
charts/sourcebot/templates/pvc.yaml (1)
2-2: Rename$volumeName— it holds the PVC name, not a volume name.
$volumeNameis computed as<fullname>-dataand is used as the PVC name passed tolookup "v1" "PersistentVolumeClaim" .... The actual PV name is read from.spec.volumeNameof that PVC. Renaming improves readability and avoids confusion when the block is revisited.✏️ Proposed rename
-{{- $volumeName := printf "%s-%s" (include "sourcebot.fullname" .) "data" -}} +{{- $pvcName := printf "%s-%s" (include "sourcebot.fullname" .) "data" -}}…and update the
lookupreferences accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/sourcebot/templates/pvc.yaml` at line 2, The variable $volumeName is misnamed — it actually holds the PVC name computed as printf "%s-%s" (include "sourcebot.fullname" .) "data"; rename it to something like $pvcName across the template (in charts/sourcebot/templates/pvc.yaml) and update every usage, specifically the lookup call lookup "v1" "PersistentVolumeClaim" ... that expects the PVC name, while the actual PV name remains .spec.volumeName; ensure all references formerly using $volumeName are changed to $pvcName for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@charts/sourcebot/templates/pvc.yaml`:
- Around line 24-34: The volumeName lookup block is incorrectly nested inside
the storageClass guard so it never runs when
.Values.sourcebot.persistence.storageClass is empty; move the lookup/volumeName
logic out of the {{- if .Values.sourcebot.persistence.storageClass }}...{{- end
}} block so it always executes, use lookup("v1","PersistentVolumeClaim",$
.Release.Namespace,$volumeName) once and store it in a variable (e.g.
$existingPvc) instead of calling lookup twice, and only render volumeName: {{
$existingPvc.spec.volumeName }} when $existingPvc and
$existingPvc.spec.volumeName are non-empty to avoid emitting an empty volumeName
field.
---
Nitpick comments:
In `@charts/sourcebot/templates/pvc.yaml`:
- Line 2: The variable $volumeName is misnamed — it actually holds the PVC name
computed as printf "%s-%s" (include "sourcebot.fullname" .) "data"; rename it to
something like $pvcName across the template (in
charts/sourcebot/templates/pvc.yaml) and update every usage, specifically the
lookup call lookup "v1" "PersistentVolumeClaim" ... that expects the PVC name,
while the actual PV name remains .spec.volumeName; ensure all references
formerly using $volumeName are changed to $pvcName for clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: abdc00d1-a6db-4e87-abf8-983ef8eb677f
📒 Files selected for processing (2)
charts/sourcebot/templates/pvc.yamlcharts/sourcebot/tests/basic_test.yaml
- Renamed `$volumeName` → `$pvcName` (line 2) — accurately reflects it holds the PVC name - Extracted `lookup` into `$existingPvc` (line 3) at the top level — called once, outside the `storageClass` guard - Moved `volumeName` block (lines 32-34) outside the `storageClass` guard — now always evaluated regardless of whether `storageClass`` is set
|
@brendan-kellam If you can take a look at this one would appreciate it, as the Helm release failure is our last deployment niggle 😄 |
1 participant
Add
helm.sh/resource-policy: keepunconditionally so the PVC surviveshelm uninstall, fix the annotations block which was previously absent when
no custom annotations were set, and use
lookupto pinvolumeNameonupgrades to prevent the PVC from being re-bound to a different PV.
Also add some test coverage.
Fixes #74
Summary by CodeRabbit
Release Notes
New Features
Tests