make velero deletable - #1176
Conversation
WalkthroughThis update increments the Kubernetes app chart version, modifies the Helm release deletion template to include the Velero release in the pre-delete hook, and updates the version mapping to reflect the new chart version and commit hash. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Pre-Delete Job
participant HelmRelease (velero)
participant RBAC
User->>Pre-Delete Job: Initiate pre-delete hook
Pre-Delete Job->>RBAC: Request permission for velero HelmRelease
RBAC-->>Pre-Delete Job: Grant get/patch permission
Pre-Delete Job->>HelmRelease (velero): Suspend HelmRelease
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
3a10ade to
99b90ff
Compare
Signed-off-by: kklinch0 <[email protected]>
99b90ff to
8fde834
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/apps/kubernetes/templates/helmreleases/delete.yaml (1)
28-45: Suspension loop is syntactically invalid – Velero (and the other releases) will not be patched
kubectl patch TYPE NAME …accepts exactly one resource name.
Passing a white-space separated list (lines 31-43) silently patches only the first item and ignores the rest, so{{ .Release.Name }}-velerowill never be suspended.A minimal fix is to iterate over the list:
- kubectl - --namespace={{ .Release.Namespace }} - patch - helmrelease - {{ .Release.Name }}-cilium - {{ .Release.Name }}-gateway-api-crds - ... - {{ .Release.Name }}-velero - -p '{"spec": {"suspend": true}}' - --type=merge --field-manager=flux-client-side-apply || true + for rel in \ + cilium gateway-api-crds csi cert-manager cert-manager-crds \ + vertical-pod-autoscaler vertical-pod-autoscaler-crds \ + ingress-nginx fluxcd-operator fluxcd gpu-operator velero; do + kubectl --namespace={{ .Release.Namespace }} \ + patch helmrelease {{ .Release.Name }}-${rel} \ + -p '{"spec": {"suspend": true}}' \ + --type=merge --field-manager=flux-client-side-apply || true + doneThis guarantees Velero (and every other release) is properly suspended during the pre-delete hook.
🧹 Nitpick comments (1)
packages/apps/kubernetes/templates/helmreleases/delete.yaml (1)
73-84: RBAC list keeps drifting – consider aggregating or templatingThe explicit
resourceNameslist was updated to include Velero (👍), but{{ .Release.Name }}-gateway-api-crdsis still missing, causing the loop above (once fixed) to fail on an RBAC “forbidden” error.Rather than manually synchronising two long lists, consider:
- Creating a single Helm template value (array) that enumerates all releases to suspend and re-using it in both the
forloop andresourceNames.- Or granting access via a label selector instead of hard-coding names.
Either approach eliminates this class of omissions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/apps/kubernetes/Chart.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/delete.yaml(2 hunks)packages/apps/versions_map(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (2)
packages/apps/kubernetes/Chart.yaml (1)
19-19: Version bump looks goodChart version incremented to
0.25.2matching the PR intent. No further issues.packages/apps/versions_map (1)
58-59: Validate pinned commit and HEAD mapping
0.25.1is now pinned toacd4663aand0.25.2points toHEAD.
Please verify that:
acd4663aexists in thekubernetesapp history and corresponds to the previous chart state.- CI/CD pipelines correctly resolve
HEADto the commit that includes this PR once merged.If either check fails, downstream deployments could fetch an unexpected chart.
2 participants
What this PR does
Release note
Summary by CodeRabbit
Bug Fixes
Chores