fix(cluster): scope cleanup to image pruning; close the GKE report follow-ups - #290
Merged
Merged
Conversation
'cluster cleanup' promised to free disk space but actually tore the whole
platform down: it deleted every ArgoCD Application, uninstalled every Helm
release in every namespace (including non-OpenFrame ones), force-stripped
finalizers from stuck Applications, and deleted the argocd/openframe
namespaces — all behind a confirmation prompt that never mentioned any of it.
That is how a routine cleanup destroyed a working install (17 apps) on
demo-local-1.
Cleanup now does exactly what its help text says: prune unused container
images inside each k3d node. Platform teardown remains where it already
lives, honestly named and properly scoped: 'app uninstall' (removes only the
app-of-apps and argo-cd releases, asks an explicit question) and 'cluster
delete'.
- drop the Application-delete / Helm-uninstall / finalizer-strip / namespace
phases and their helpers from the cluster service, along with the
ApplicationCleaner injection in the cleanup command
- reword the confirmation prompt to state what cleanup does ('Prune unused
container images... Installed apps are not touched')
- fix the help text and the --force description (no more 'aggressive
cleanup'), update docs
- delete the tests of the removed phases; rescope the summary/result tests
The GKE lifecycle report (addendum 2026-07-28) flagged a contradiction: the 'Cluster Deleted' box printed 'RESOURCES: Cleaned up' directly under the orphan-disk warning listing PVC-provisioned disks that survived the destroy. Those disks live outside terraform state; the sweep right above the box is what reports or (with consent) deletes them. The RESOURCES row now claims only what the path verified: k3d keeps 'Cleaned up' (delete removes everything the cluster owned), cloud types say 'Terraform-managed destroyed (leftovers, if any, reported above)'. Pinned by a test. Also align the cluster group help with cleanup's new image-prune-only scope.
…orm.log Long cloud operations left the terminal scrollback as their only record. The engine now tees terraform's raw JSON-UI stream into terraform.log next to the workspace state — each run appends under a timestamped header — and a failed apply/destroy names the log path in its error. Best-effort: an unwritable directory never blocks the operation.
…ification report Addresses the design findings of the GKE lifecycle verification report (the retry hint for failed infracost downloads and the GCP project picker were already implemented): Project APIs are no longer terraform-managed. As google_project_service resources, every cluster workspace in a shared project claimed ownership of the same project-level toggles and every destroy planned their removal. The CLI now enables the required APIs (compute, container) with one idempotent 'gcloud services enable' before terraform runs — create-time step, owned by no cluster's state. A denied enable falls back to an enabled-state probe so deploy-only identities on an already-configured project still proceed; genuinely missing APIs stop the create with the exact manual command. Dry-run is untouched: plan keeps zero project side effects. Every display of a regional (--ha) cluster shows the per-zone math. 'Nodes: 3' that silently provisioned 9 (regional counts are per zone) now reads '3 per zone × 3 zones = 9 total (regional)' in the configuration summary and wizard; the workspace record gains an HA field so cluster list/status report the real node total for regional clusters. The cost warning now advertises --spot for test clusters (typically 60-90% off the node cost) whenever a cloud config doesn't already use it, and the summaries show a Spot row when it does. Template propagation was verified: GKE passes spot=var.spot, EKS maps it to capacity_type=SPOT.
The old text pitched the tool against shell scripts it replaced long ago and praised its own design instead of telling the user what the commands do. The new help states the actual surface: local k3d and Terraform-backed GKE/EKS provisioning, the app-of-apps install, the typical local and cloud flows, and what each command group covers — including cleanup's real (image-prune-only) scope and the plan/confirm safety of cloud creates.
- GKE workflow: project APIs are enabled in preflight via gcloud (not terraform), the dry-run example no longer lists google_project_service and reflects the smaller zonal plan, the cluster is zonal by default (--ha for regional, with the per-zone node math spelled out), --spot is called out next to the cost note, and delete now describes the disk-release phase and the consented orphan sweep - Cloud clusters reference: --ha and --spot documented, delete's release-and-sweep behavior explained, terraform.log added to the workspace contents and troubleshooting - Architecture decisions (D8): terraform.log listed among workspace files - Reference overview + README: cleanup described as image pruning only
'dev (<commit>) built on <date>' answered which build but not on what — the Go version and os/arch are the first questions of any bug report about a downloaded release. The version stays the first whitespace token, which is the contract selfupdate's rollback parses for its label; the suffix rides after the date. Pinned by the root-command test.
--version now answers 'which terraform/helm/k3d/mkcert/infracost does this build install, and which ArgoCD chart does it deploy' — straight from the single sources of truth (the PinnedTool definitions and the ArgoCD chart constant), so the block can never drift from what the installers actually pin. The version string keeps its first-token contract for selfupdate's rollback label parsing.
The comments explained themselves; the M/S index tags pointed at an external report the next reader does not have. The prose keeps the why — the tags go.
|
Warning Review limit reached
Next review available in: 39 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe PR narrows cluster cleanup to unused node-image pruning, adds GKE API preflight and HA node accounting, persists Terraform operation logs, updates cluster UI output, and expands CLI runtime and dependency version details. ChangesCluster cleanup scope
GKE provisioning
Terraform logging
Cluster UI
CLI metadata
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CreateCluster
participant gcloud
participant TerraformEngine
participant Workspace
CreateCluster->>gcloud: Enable required GKE APIs
gcloud-->>CreateCluster: Return API status or remediation error
CreateCluster->>TerraformEngine: Apply cluster configuration
TerraformEngine->>Workspace: Append Terraform output
TerraformEngine-->>CreateCluster: Return apply result and HA record
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
The path is a CLI-managed workspace directory — the same trust boundary as the terraform state written next to it — joined with a constant filename.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
internal/cluster/service_test.go (1)
204-212: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the returned
CleanupResult, not only the error.
cleanupK3dClusteralways returns a nil error. The partial-cleanup contract lives inCleanupResult.FailuresandNodesPruned. This test discards the result, so a regression that records zero nodes or drops failures still passes.♻️ Proposed test strengthening
- _, err := service.CleanupCluster(context.Background(), "test-cluster", models.ClusterTypeK3d, false) + result, err := service.CleanupCluster(context.Background(), "test-cluster", models.ClusterTypeK3d, false) if err != nil { t.Errorf("CleanupCluster should not error: %v", err) } + if result.Removed() != result.NodesPruned { + t.Errorf("Removed() must report NodesPruned; got %d and %d", result.Removed(), result.NodesPruned) + }Add a separate case that asserts a cloud cluster type returns the
cluster deleteerror.🤖 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 `@internal/cluster/service_test.go` around lines 204 - 212, Strengthen TestClusterService_CleanupCluster by capturing the returned CleanupResult and asserting the expected NodesPruned and Failures values for the K3d cleanup path, rather than checking only the error. Add a separate test case for a cloud cluster type that verifies CleanupCluster returns the cluster delete error.
🤖 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 `@cmd/cluster/cleanup.go`:
- Around line 20-25: Correct the sentence in the command’s Long help text by
adding the missing relative pronoun so it clearly states that only images with
no container references are removed; leave the surrounding cleanup guidance
unchanged.
In `@cmd/root_test.go`:
- Around line 117-138: Add assertions in the root command version test alongside
the existing pinned-dependency checks to require runtime.Version(), mkcert’s
pinned version, and Infracost’s pinned version in cmd.Version. Reference the
established dependency source symbols used by cmd/root.go, and preserve the
existing output-format and dependency assertions.
In `@cmd/root.go`:
- Around line 146-149: Update the command help text near the
interactive/non-interactive description to remove the promise that cloud deletes
“clean up after themselves.” Describe that cloud deletion handles
Terraform-managed resources while noting that some resources or leftovers may
remain.
In `@internal/cluster/models/flags.go`:
- Line 107: Update ShowCleanupSummary to remove the --force suggestion from the
partial-failure cleanup retry hint, since the flag only skips confirmation and
does not alter pruning behavior. Replace it with a plain cleanup re-run hint or
retain only the cluster deletion hint.
In `@internal/cluster/providers/terraform/engine.go`:
- Around line 151-156: Replace the io.MultiWriter call in the Terraform log
setup with a best-effort tee that always writes progress output, ignores
log-sink errors, and disables the file sink after its first write failure. Keep
ApplyJSON and DestroyJSON successful when terraform.log becomes unavailable,
while preserving the existing close callback and returned log name.
---
Nitpick comments:
In `@internal/cluster/service_test.go`:
- Around line 204-212: Strengthen TestClusterService_CleanupCluster by capturing
the returned CleanupResult and asserting the expected NodesPruned and Failures
values for the K3d cleanup path, rather than checking only the error. Add a
separate test case for a cloud cluster type that verifies CleanupCluster returns
the cluster delete error.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: db975920-f8cb-4bf3-a7c9-ff84cdf597f2
📒 Files selected for processing (31)
README.mdcmd/cluster/cleanup.gocmd/cluster/cluster.gocmd/root.gocmd/root_test.godocs/architecture/decisions.mddocs/getting-started/cloud-clusters.mddocs/getting-started/first-steps.mddocs/getting-started/gke-workflow.mddocs/reference/architecture/overview.mdinternal/cluster/cleanup_finalizers_test.gointernal/cluster/cleanup_helm_test.gointernal/cluster/cleanup_result_test.gointernal/cluster/cleanup_safety_test.gointernal/cluster/models/cleanup.gointernal/cluster/models/cluster.gointernal/cluster/models/flags.gointernal/cluster/providers/gke/gcloud_command_contract_test.gointernal/cluster/providers/gke/provider.gointernal/cluster/providers/gke/template_guard_test.gointernal/cluster/providers/gke/templates/main.tfinternal/cluster/providers/terraform/engine.gointernal/cluster/providers/terraform/engine_test.gointernal/cluster/providers/terraform/workspace.gointernal/cluster/service.gointernal/cluster/service_test.gointernal/cluster/ui/cleanup_summary_test.gointernal/cluster/ui/operations.gointernal/cluster/ui/operations_test.gointernal/cluster/ui/prompts.gointernal/cluster/ui/wizard_steps.go
💤 Files with no reviewable changes (4)
- internal/cluster/cleanup_finalizers_test.go
- internal/cluster/cleanup_helm_test.go
- internal/cluster/cleanup_result_test.go
- internal/cluster/cleanup_safety_test.go
- cleanup help: 'Only images no container references' was missing its relative pronoun — now 'Only images that no container references are removed', split into plain sentences - cleanup partial-failure hint: stop suggesting a --force re-run; the flag only skips the confirmation prompt, so it implied a more aggressive retry that doesn't exist - root help: cloud deletes 'clean up after themselves' overstated it — they destroy the terraform-managed resources and report leftovers they could not remove - root version test: also pin the Go toolchain, mkcert and infracost lines, so dropping any advertised field fails the test
io.MultiWriter propagates a sink error, and exec.Cmd returns a stdout-writer error from Wait — so a disk filling up after the log file opened would report a completed apply/destroy as FAILED while terraform had actually changed resources. The log stream now goes through a best-effort tee: progress output is authoritative, the file sink is dropped on its first write failure and never re-enabled. The log records the operation; it must not decide its outcome.
ivan-flamingo
previously approved these changes
Aug 7, 2026
--version output grew a pinned-dependencies block, so it is multi-line now. The test parsed it with a bare 'cut -d" " -f1', which cuts EVERY line and made the comparison fail on all platforms. The version contract is unchanged — first token of the first line (same as selfupdate's rollback label parser) — so the checks now take head -n1 before cut. The release smoke test greps the whole output and needed no change.
ivan-flamingo
approved these changes
Aug 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
An interactive
cluster cleanupon demo-local-1 destroyed the entire platform(17 ArgoCD applications, all Helm releases, namespaces) while its help text
promised "freeing disk space". Investigation showed this was not a bug in
garbage detection — cleanup had no notion of "unused" at all: it was a full
platform teardown hiding behind a disk-cleanup name, duplicating the honestly
named
app uninstall.What
Cleanup is now exactly what it says (
9d94a1a)cluster cleanuponly prunes unused container images on k3d nodes(
crictl rmi --prune); the Application-delete / Helm-uninstall /finalizer-strip / namespace phases are removed
touched"); help,
--forcedescription and docs no longer promise or hidea teardown
app uninstall(scoped to the app-of-apps and argo-cd releases) and
cluster deleteSummary by CodeRabbit
New Features
cluster cleanupnow safely prunes unused container images without affecting installed applications or namespaces.Documentation