Skip to content

feat: clean up distrobox exports/entries even if container is already deleted#2062

Open
iTrooz wants to merge 6 commits into89luca89:nextfrom
iTrooz:next_delete_distrobox_desktop_first
Open

feat: clean up distrobox exports/entries even if container is already deleted#2062
iTrooz wants to merge 6 commits into89luca89:nextfrom
iTrooz:next_delete_distrobox_desktop_first

Conversation

@iTrooz
Copy link
Copy Markdown

@iTrooz iTrooz commented Apr 20, 2026

See #2059

@iTrooz
Copy link
Copy Markdown
Author

iTrooz commented Apr 20, 2026

I also added a failsafe on cleanup() because I accidentally triggered it with an empty container name during tests, and ended up deleting a lot of other files on my computer. Tell me if you'd prefer me to remove it :)

@iTrooz iTrooz force-pushed the next_delete_distrobox_desktop_first branch from db0ba66 to a48ad88 Compare April 20, 2026 09:14
@balanza balanza requested a review from Copilot April 22, 2026 12:25
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds behavior to distrobox rm to also clean up exported binaries/desktop entries for explicitly requested containers even when the container no longer exists, aligning with the “Remove exported apps” workflow discussed in #2059.

Changes:

  • Track explicitly requested container names that were not found/removed and run cleanup for them.
  • Add a small slice helper to remove handled names from the explicit list.
  • Add a guard in cleanup() for empty container names.
Comments suppressed due to low confidence (1)

pkg/commands/rm.go:88

  • This fmt.Printf call lacks a trailing newline, which can make output from multiple deletions run together and be harder to read. Consider adding "\n" (and/or routing through the logger once available).
			//nolint:forbidigo // waiting for the logger implementation
			fmt.Printf("error deleting %s: %s", currentDistrobox.Name, err)
		}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/commands/rm.go Outdated
Comment on lines +92 to +96
// Clean up exported files of all remaining explicitely requested distroboxes,
// even if the container doesn't exist anymore
for _, containerName := range explicitelyRequested {
c.cleanup(ctx, userHome, containerName)
}
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New behavior to run cleanup for explicitly requested but non-existent containers isn’t covered by tests. Consider adding a unit test to ensure it deletes only the expected exported artifacts and does not act on unsafe container names (e.g., empty, path traversal, glob chars).

Copilot uses AI. Check for mistakes.
Comment thread pkg/commands/rm.go Outdated
Comment thread pkg/commands/rm.go Outdated
Comment thread pkg/commands/rm.go Outdated
Comment thread pkg/commands/rm.go Outdated
Comment thread pkg/commands/rm.go Outdated
Comment thread pkg/commands/rm.go Outdated
@balanza balanza self-assigned this Apr 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants