feat: clean up distrobox exports/entries even if container is already deleted#2062
feat: clean up distrobox exports/entries even if container is already deleted#2062iTrooz wants to merge 114 commits into
Conversation
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
* build(deps): add urfave/cli Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com> * chore: add gitignore Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com> * ci: add golangci configuration Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com> * build(Makefile): add Makefile Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com> * feat: add main package and distrobox cmd Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com> --------- Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
* feat: add containermanager package and docker provider Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com> * feat: add list command Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com> * feat: add root command Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com> * feat: add list command Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com> * feat: wire-up root command in main Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com> * refactor(docker): use format json Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com> --------- Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
* chore: disable mdlint (89luca89#6) * resolve lint issues Some issues where detected by the CI once run: * use of `fmt.Printf` forbidden by pattern `^(fmt\.Print(|f|ln)|print|println)$` (forbidigo) * File is not properly formatted (goimports) * Magic number: 12, in <condition> detected (mnd) * error-format: fmt.Errorf can be replaced with errors.New (perfsprint) --------- Co-authored-by: Alessio Biancalana <alessio@dottorblaster.it>
Bumps [actions/checkout](https://github.com/actions/checkout) from 5 to 6. - [Release notes](https://github.com/actions/checkout/releases) - [Commits](actions/checkout@v5...v6) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
* feat(cli): validate sudo (sudo -v) in beforeCommand if --root is set Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com> * feat(docker): add root and sudoCommand and execute commands with sudo if root is set Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com> * feat(cli): add --root description and -r alias Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com> --------- Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
* add GenerateEntryCommand package The package mimics the actual distrobox-generate-entry shell command that installs a desktop entry file so that the host's desktop environment can render the appropriate application icon. This first implementantion scaffolds the command package and implements the simplest scenario for a single container. * delete entry If the `delete` flag is provided, the command remove the desktop entry if present. Deleting a non-existing item does not raise an error. * generate entries for all containers With `opts.All` we implement an abstraction over the `GenerateEntryCommand` that iterates across all the containers to generate or delete the relative desktop entries. `ListCommand` is used to fetch the list of containers. * mount the generate-entry cli command Mount the command so that is reachable from the CLI. Define parameters with type and basic validation. Depending on the flag `--all`, the appropriate set of options is provided to the `GenerateEntryCommand.Execute()` method. If `--all` is set, `container-name` and `icon` arguments will be ignored.
* embed shell scripts to execute inside the distrobox The command init, export and host-exec must be accessible from inside the distrobox container. When needed, the embedded scripts will be written to a host directory and then mounted as volume on the created container. The host directory is determined by configuration and environment variables. The script will be not converted to GO to keep then architecture-agnostic. * add Create to the Docker container manager The function will compose argument string to append to the `docker` command. If `DryRun=true`, the command is just printed on screen and not executed. * add Create command package * mount cli command
* feat: add config Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com> * build(deps): add testify Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com> * test(config): add config loading test Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com> --------- Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
* feat(enter): rewrite in go * chore: make InspectContainer public * chore: use userenv package for user's environment * chore: export InspectResult type for convenience * fix: rename containerInspect to inpectOutput * fix: use userenv inside generateEnterCommand * fix(enter): more userenv * chore: add some unit tests for buildContainerPath and buildCommandArgs
* read desktop entry directory and distrobox path from the global config * implement entry generation requested from user
* remove unused ID field * add a test for include order
* scaffold assemble command * mount assemble command * implement create item
Manifest can either be file or remote URLs. In the latter case, the content is fetched into a temporary file; then the parsing follows as usual. Note that the HTTP fetch has no constraint on protocol to use nor an allowed list of URLs, just like in the original `assemble` command. https://github.com/89luca89/distrobox/blob/main/distrobox-assemble#L240 ``` if command -v curl > /dev/null 2>&1; then download="curl --connect-timeout 3 --retry 1 -sLo" elif command -v wget > /dev/null 2>&1; then download="wget --timeout=3 --tries=1 -qO" ``` The only difference with the original implementation is that the file input is checked to be a valid URL.
* feat: rewrite rm in go * fix: declare yes and no as string slices
* move the Propter under screen package * add color formatters * add progress This module allows for printing a sequence of lines with check marks. Useful to show progress on a long running action (example: container init). * implement progess on create * implement progress on assemble * implement progress on enter * implement color * move internal/screen to pkg/ui * add printer * implement printer * fix progress finalize method * fix progress implementation for enter command
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
Replace helper functions (lastString, lastBool, allStrings, splitString) with a single loop that fetches ValueWithShadows() once per key. This avoids repeated calls to the ini library for each field. Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
…ect-binary fix(providers): execute correct binary for podman-launcher provider
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
|
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 :) |
db0ba66 to
a48ad88
Compare
There was a problem hiding this comment.
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.
|
Hello ! Any news ? Changes I need to make ? |
|
Hi, Right now, we are focusing on making Distrobox v2 stable, which means having feature parity with Distrobox v1. That implies that, for the moment, we are not considering new features or bug fixes besides those that are trivial to merge and do not require much brain power from us. On this PR, we are not convinced by the implementation, but at the same time, we cannot give you the level of feedback you deserve. Let's postpone this contribution until Distrobox v2 is stable. Closing for now. |
|
Would you mind reopening & drafting this PR instead, so it's still on my feed and I remember to come back to it later ? Thanks |
2db8252 to
ab1b3ac
Compare
See #2060