feat(bundler): drop generated undeploy.sh; delegate to helm uninstall#1095
Conversation
… uninstall The helm deployer no longer emits undeploy.sh. Bundle teardown delegates to the deployer-native uninstall path (helm uninstall, ArgoCD app deletion). The script had accumulated complexity across multiple hardening passes chasing CRD lifecycle, finalizer resolution, and multi-tenancy safety — problems Helm itself deliberately avoids and that purpose-built deployers handle more safely. AICR's role is to generate validated design-time bundles, not to be the deployer. Migration: | Before | After (helm) | | ------------------------------- | --------------------------------------------- | | ./undeploy.sh | helm uninstall <release> -n <namespace> | | ./undeploy.sh --delete-pvcs | kubectl -n <ns> delete pvc --all | | ./undeploy.sh --keep-namespaces | (default; Helm doesn't delete namespaces) | | Before | After (argocd / argocd-helm) | | ------------- | --------------------------------------------- | | ./undeploy.sh | kubectl -n argocd delete application <parent> | See the new "Bundle Uninstall" section in docs/user/cli-reference.md for full per-deployer walkthroughs, CRD/PVC handling, and stuck-release recovery. Closes #671
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (4)
📝 WalkthroughWalkthroughThis PR removes generation of top-level undeploy.sh from Helm bundles, deletes the undeploy template and related undeploy tests/helpers, adds Generator.Generate logic to remove stale top-level undeploy.sh on regeneration, changes README generation to derive reverse uninstall order from emitted localformat folders, updates templates and generated testdata to instruct deployer-native uninstall flows, and adjusts cross-package docs and tests accordingly. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Coverage Report ✅
Coverage BadgeMerging this branch will decrease overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. |
…dback Addresses three valid review concerns from @yuanchen8911 and matching inline feedback from coderabbit: 1. README uninstall list missed auxiliary releases. The Uninstall block in the generated README.md ranged over recipe components, but deploy.sh installs every NNN-* folder including injected *-pre / *-post auxiliaries. Threaded localformat.Write's folder list through to the README template via a new releaseRef type; the template now enumerates every emitted release in reverse-install order. Verified testdata/mixed_with_pre and testdata/mixed_gpu_operator now include the *-pre / *-post releases. Folder gains a Namespace field so the caller doesn't have to re-derive it. 2. Stale undeploy.sh survived bundle regeneration. localformat.Write only prunes NNN-* folders; regenerating over a pre-PR bundle left an executable, unchecksummed undeploy.sh contradicting the new README. Generate() now explicitly removes any top-level undeploy.sh up front, ignoring ENOENT but propagating other errors. Covered by happy-path and error-path tests. 3. ArgoCD uninstall doc claimed AICR sets resources-finalizer on every Application — false; the argocd templates set no finalizer. Rewrote the argocd walkthrough to recommend `argocd app delete --cascade` or a manual `kubectl patch` to add the finalizer before delete. Also corrected the argocd-helm wording: ArgoCD with a Helm source uses helm template + native pruning, not `helm uninstall`. Added flux (HelmRelease delete) and helmfile (`helmfile destroy`) subsections so the section covers every deployer AICR generates. Tests: - New TestReverseReleases replaces TestReverseComponents (dead helper removed alongside its consumer). - New TestGenerate_RemovesStaleUndeployScript + TestGenerate_StaleUndeployRemovalErrorPropagates lock in the stale-removal behavior on both paths. - Regenerated 6 golden testdata bundles for the new README shape. Coverage on pkg/bundler/deployer/helm: 84.2% (main) -> 84.0% (-0.2%).
…nd 2 Two follow-up nits from coderabbit on commit 125f49b: 1. `argocd app delete --cascade=foreground` is not valid CLI syntax; `--cascade` is a boolean and foreground propagation is set via `--propagation-policy`. Fixed both occurrences in docs/user/cli-reference.md (argocd + argocd-helm walkthroughs) to use `--cascade --propagation-policy foreground`. 2. README.md.tmpl emitted adjacent fenced code blocks without a blank line between them when the bundle has multiple releases (e.g. `mixed_with_pre`, `mixed_gpu_operator`), violating markdownlint MD031. Tightened the range body's whitespace control so each `helm uninstall` block is separated by exactly one blank line, with exactly one blank line between the final block and the next paragraph. Regenerated all 6 golden bundles.
Summary
Removes the generated
undeploy.shfrom helm bundles and delegates teardown to the deployer-native uninstall path (helm uninstall, ArgoCD app deletion). Rewritesdocs/user/cli-reference.mdUndeploy Script Behavioras aBundle Uninstallsection with per-deployer walkthroughs.Motivation / Context
Per the issue,
undeploy.shaccumulated complexity across #253, #282, #364, #416, #477, #561, #602, #661 chasing CRD lifecycle, finalizer resolution, and multi-tenancy safety — problems Helm itself deliberately avoids and that purpose-built deployers handle more safely. AICR's role is to generate validated design-time bundles, not to be the deployer.The implementation matches the gating decisions made during planning:
tools/utility — verified that nothing in CI / Makefile / chainsaw / KWOK /tools/e2eactually invokesundeploy.sh, so the "minimal scope" choice maps to zero migration targetFixes: #671
Related: #610, #602, #661, #669
Type of Change
Component(s) Affected
pkg/bundler,pkg/component/*)docs/,examples/)Implementation Notes
Deviation from issue AC #3 (tools/ utility): The issue's acceptance criteria say "tools/ provides equivalent teardown for developer workflows," but a repo-wide sweep found that nothing in CI (
.github/workflows/),Makefile,tools/scripts, chainsaw tests, KWOK workflows, or developer docs (DEVELOPMENT.md,CONTRIBUTING.md) actually invokesundeploy.sh. The only existing chainsaw reference is a negative assertion that helmfile bundles don't leakundeploy.sh. With the "Only what CI/KWOK/Kind exercise" scope choice, the resulting tool would be empty. Skipping thetools/utility entirely — surfacing this discrepancy here.Chainsaw assertion: Dropped the
undeploy.sh leaked into helmfile bundlecheck fromtests/chainsaw/cli/bundle-helmfile/chainsaw-test.yaml. After this changeundeploy.shis no longer generated by any deployer, so the negative assertion is universally true and adds noise.Golden testdata regenerated to reflect the new bundle shape — no more
undeploy.shgolden files, updatedREADME.mdUninstall section, updateddeploy.shpre-flight error hint.Helper function cleanup:
uniqueNamespaces(only used by undeploy generation) removed.reverseComponentsretained — still used by README.md's per-component manual-uninstall block.Testing
go test -race ./pkg/bundler/...— all packages passgolangci-lint run -c .golangci.yaml ./...— 0 issuesyamllint— clean on the modified chainsaw testCoverage delta (
pkg/bundler/deployer/helm): 84.2% → 83.7% (-0.5%, at the threshold). The drop comes from removing the 14TestUndeployScript_*shell-behavior tests that exercised the deleted template. Absolute level still well above the 70% project floor.Pre-existing
pkg/trustfailures in the local run are network-related (sandbox can't reachtuf-repo-cdn.sigstore.dev) and reproduce onorigin/main; unrelated to this PR.Risk Assessment
Rollout notes: This is a breaking change for any user who scripted against
./undeploy.shin a generated helm bundle. Migration:./undeploy.shhelm uninstall <release> -n <namespace>./undeploy.sh --delete-pvcskubectl -n <ns> delete pvc --all./undeploy.sh --keep-namespaces./undeploy.shkubectl -n argocd delete application <parent>Full per-deployer walkthrough (CRD handling, PVC handling, stuck-release recovery) is in
docs/user/cli-reference.md#bundle-uninstall. The bundle'sREADME.mdnow embeds the renderedhelm uninstallcommands per component and links to the doc section.Checklist
make testwith-race)make lint)git commit -S)