From ef0b302fb6bb7b14ac93a7547a76c80ed1bb7d25 Mon Sep 17 00:00:00 2001 From: Debarshi Ray Date: Mon, 23 Mar 2026 23:37:57 +0100 Subject: [PATCH 1/3] cmd/list, pkg/podman: Simplify code by removing a function parameter The getContainers() function originated as listContainers(), when it invoked 'podman ps --filter label=...' through podman.GetContainers() separately for each label used to identify Toolbx containers. The label was specified using the 'args' parameter. This changed in commit 2369da5d31830e5c when getContainers() started invoking 'podman ps' only once through podman.GetContainers() to get all available containers, with the 'args' parameter set to a fixed value. https://github.com/containers/toolbox/pull/1775 --- src/cmd/list.go | 4 ++-- src/pkg/podman/podman.go | 6 ++---- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/cmd/list.go b/src/cmd/list.go index eb1e96d9e..578d32d10 100644 --- a/src/cmd/list.go +++ b/src/cmd/list.go @@ -108,8 +108,8 @@ func list(cmd *cobra.Command, args []string) error { func getContainers() ([]podman.Container, error) { logrus.Debug("Fetching all containers") - args := []string{"--all", "--sort", "names"} - containers, err := podman.GetContainers(args...) + + containers, err := podman.GetContainers() if err != nil { logrus.Debugf("Fetching all containers failed: %s", err) return nil, errors.New("failed to get containers") diff --git a/src/pkg/podman/podman.go b/src/pkg/podman/podman.go index 26ff02781..510a22d7f 100644 --- a/src/pkg/podman/podman.go +++ b/src/pkg/podman/podman.go @@ -165,16 +165,14 @@ func ContainerExists(container string) (bool, error) { // GetContainers is a wrapper function around `podman ps --format json` command. // -// Parameter args accepts an array of strings to be passed to the wrapped command (eg. ["-a", "--filter", "123"]). -// // Returned value is a slice of Containers. // // If a problem happens during execution, first argument is nil and second argument holds the error message. -func GetContainers(args ...string) (*Containers, error) { +func GetContainers() (*Containers, error) { var stdout bytes.Buffer logLevelString := LogLevel.String() - args = append([]string{"--log-level", logLevelString, "ps", "--format", "json"}, args...) + args := []string{"--log-level", logLevelString, "ps", "--all", "--format", "json", "--sort", "names"} if err := shell.Run("podman", nil, &stdout, nil, args...); err != nil { return nil, err From 7b7576c34cd33a91b6825b57ed5977171a1f706d Mon Sep 17 00:00:00 2001 From: Debarshi Ray Date: Mon, 23 Mar 2026 23:55:48 +0100 Subject: [PATCH 2/3] cmd/list, pkg/podman: Make podman.GetContainers() filter ... not getContainers(). This is a step towards eliminating the code in getContainers() by doing everything in podman.GetContainers(). This removes one unnecessary layer of code. The getContainers() function originated as listContainers(), when it invoked 'podman ps --filter label=...' through podman.GetContainers() separately for each label used to identify Toolbx containers, and then joined and sorted the results. This changed in commit 2369da5d31830e5c when getContainers() started invoking 'podman ps' only once through podman.GetContainers() to get all available containers, and then filtered them itself based on the labels. Before this change in commit 2369da5d31830e5c, it probably made some sense to keep podman.GetContainers() only as a thin wrapper to invoke 'podman ps' with different options, and to do everything else in getContainers(). However, since then more is being done inside the podman package (eg., unmarshalling the 'podman ps' JSON in commit d0323027e0e423a4), and getContainers() is the only caller of podman.GetContainers(). Therefore, it looks awkward to have the code to get all Toolbx containers split across two functions in different packages. https://github.com/containers/toolbox/pull/1775 --- src/cmd/list.go | 5 ++--- src/pkg/podman/podman.go | 11 +++++++++-- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/cmd/list.go b/src/cmd/list.go index 578d32d10..35aa29814 100644 --- a/src/cmd/list.go +++ b/src/cmd/list.go @@ -118,9 +118,8 @@ func getContainers() ([]podman.Container, error) { var toolboxContainers []podman.Container for containers.Next() { - if container := containers.Get(); container.IsToolbx() { - toolboxContainers = append(toolboxContainers, container) - } + container := containers.Get() + toolboxContainers = append(toolboxContainers, container) } return toolboxContainers, nil diff --git a/src/pkg/podman/podman.go b/src/pkg/podman/podman.go index 510a22d7f..7c2a6388f 100644 --- a/src/pkg/podman/podman.go +++ b/src/pkg/podman/podman.go @@ -163,7 +163,7 @@ func ContainerExists(container string) (bool, error) { return true, nil } -// GetContainers is a wrapper function around `podman ps --format json` command. +// GetContainers is a wrapper function around `podman ps --format json` command that returns all Toolbx containers // // Returned value is a slice of Containers. // @@ -184,7 +184,14 @@ func GetContainers() (*Containers, error) { return nil, err } - return &Containers{containers, 0}, nil + var toolbxContainers []containerPS + for _, container := range containers { + if container.IsToolbx() { + toolbxContainers = append(toolbxContainers, container) + } + } + + return &Containers{toolbxContainers, 0}, nil } // GetImages is a wrapper function around `podman images --format json` command that returns all Toolbx images From 966c823d72be1d4bbd5cf34ac8bf75445345b93e Mon Sep 17 00:00:00 2001 From: Debarshi Ray Date: Tue, 24 Mar 2026 15:59:19 +0100 Subject: [PATCH 3/3] cmd, pkg/podman: Remove getContainers() https://github.com/containers/toolbox/pull/1775 --- src/cmd/completion.go | 20 +++++++++++++++---- src/cmd/list.go | 38 ++++++++++++------------------------- src/cmd/rm.go | 11 ++++++++--- src/cmd/run.go | 11 ++++++++--- src/pkg/podman/container.go | 8 ++++++++ 5 files changed, 52 insertions(+), 36 deletions(-) diff --git a/src/cmd/completion.go b/src/cmd/completion.go index e5c4eea7e..0e6fb1fe4 100644 --- a/src/cmd/completion.go +++ b/src/cmd/completion.go @@ -74,9 +74,15 @@ func completionCommands(cmd *cobra.Command, _ []string, _ string) ([]string, cob } func completionContainerNames(_ *cobra.Command, _ []string, _ string) ([]string, cobra.ShellCompDirective) { + logrus.Debug("Getting all containers") + var containerNames []string - if containers, err := getContainers(); err == nil { - for _, container := range containers { + + if containers, err := podman.GetContainers(); err != nil { + logrus.Debugf("Getting all containers failed: %s", err) + } else { + for containers.Next() { + container := containers.Get() name := container.Name() containerNames = append(containerNames, name) } @@ -90,9 +96,15 @@ func completionContainerNamesFiltered(cmd *cobra.Command, args []string, _ strin return nil, cobra.ShellCompDirectiveNoFileComp } + logrus.Debug("Getting all containers") + var containerNames []string - if containers, err := getContainers(); err == nil { - for _, container := range containers { + + if containers, err := podman.GetContainers(); err != nil { + logrus.Debugf("Getting all containers failed: %s", err) + } else { + for containers.Next() { + container := containers.Get() name := container.Name() skip := false for _, arg := range args { diff --git a/src/cmd/list.go b/src/cmd/list.go index 35aa29814..2646cb17c 100644 --- a/src/cmd/list.go +++ b/src/cmd/list.go @@ -82,7 +82,7 @@ func list(cmd *cobra.Command, args []string) error { } var images []podman.Image - var containers []podman.Container + var containers *podman.Containers var err error if lsImages { @@ -96,9 +96,12 @@ func list(cmd *cobra.Command, args []string) error { } if lsContainers { - containers, err = getContainers() + logrus.Debug("Getting all containers") + + containers, err = podman.GetContainers() if err != nil { - return err + logrus.Debugf("Getting all containers failed: %s", err) + return errors.New("failed to get containers") } } @@ -106,25 +109,6 @@ func list(cmd *cobra.Command, args []string) error { return nil } -func getContainers() ([]podman.Container, error) { - logrus.Debug("Fetching all containers") - - containers, err := podman.GetContainers() - if err != nil { - logrus.Debugf("Fetching all containers failed: %s", err) - return nil, errors.New("failed to get containers") - } - - var toolboxContainers []podman.Container - - for containers.Next() { - container := containers.Get() - toolboxContainers = append(toolboxContainers, container) - } - - return toolboxContainers, nil -} - func listHelp(cmd *cobra.Command, args []string) { if utils.IsInsideContainer() { if !utils.IsInsideToolboxContainer() { @@ -146,7 +130,7 @@ func listHelp(cmd *cobra.Command, args []string) { } } -func listOutput(images []podman.Image, containers []podman.Container) { +func listOutput(images []podman.Image, containers *podman.Containers) { if len(images) != 0 { writer := tabwriter.NewWriter(os.Stdout, 0, 0, 2, ' ', 0) fmt.Fprintf(writer, "%s\t%s\t%s\n", "IMAGE ID", "IMAGE NAME", "CREATED") @@ -165,11 +149,11 @@ func listOutput(images []podman.Image, containers []podman.Container) { writer.Flush() } - if len(images) != 0 && len(containers) != 0 { + if len(images) != 0 && containers.Len() != 0 { fmt.Println() } - if len(containers) != 0 { + if containers.Len() != 0 { const boldGreenColor = "\033[1;32m" const defaultColor = "\033[0;00m" // identical to resetColor, but same length as boldGreenColor const resetColor = "\033[0m" @@ -194,7 +178,9 @@ func listOutput(images []podman.Image, containers []podman.Container) { fmt.Fprintf(writer, "\n") - for _, container := range containers { + for containers.Next() { + container := containers.Get() + isRunning := false if podman.CheckVersion("2.0.0") { status := container.Status() diff --git a/src/cmd/rm.go b/src/cmd/rm.go index 5b6246ac7..d4d0aac68 100644 --- a/src/cmd/rm.go +++ b/src/cmd/rm.go @@ -24,6 +24,7 @@ import ( "github.com/containers/toolbox/pkg/podman" "github.com/containers/toolbox/pkg/utils" + "github.com/sirupsen/logrus" "github.com/spf13/cobra" ) @@ -67,12 +68,16 @@ func rm(cmd *cobra.Command, args []string) error { } if rmFlags.deleteAll { - toolboxContainers, err := getContainers() + logrus.Debug("Getting all containers") + + toolboxContainers, err := podman.GetContainers() if err != nil { - return err + logrus.Debugf("Getting all containers failed: %s", err) + return errors.New("failed to get containers") } - for _, container := range toolboxContainers { + for toolboxContainers.Next() { + container := toolboxContainers.Get() containerID := container.ID() if err := podman.RemoveContainer(containerID, rmFlags.forceDelete); err != nil { fmt.Fprintf(os.Stderr, "Error: %s\n", err) diff --git a/src/cmd/run.go b/src/cmd/run.go index 1de96c7a4..ed421aa68 100644 --- a/src/cmd/run.go +++ b/src/cmd/run.go @@ -193,13 +193,16 @@ func runCommand(container string, return err } - containers, err := getContainers() + logrus.Debug("Getting all containers") + + containers, err := podman.GetContainers() if err != nil { + logrus.Debugf("Getting all containers failed: %s", err) err := createErrorContainerNotFound(container) return err } - containersCount := len(containers) + containersCount := containers.Len() logrus.Debugf("Found %d containers", containersCount) if containersCount == 0 { @@ -228,7 +231,9 @@ func runCommand(container string, } else if containersCount == 1 && defaultContainer { fmt.Fprintf(os.Stderr, "Error: container %s not found\n", container) - container = containers[0].Name() + containers.Next() + containerObj := containers.Get() + container = containerObj.Name() fmt.Fprintf(os.Stderr, "Entering container %s instead.\n", container) fmt.Fprintf(os.Stderr, "Use the 'create' command to create a different Toolbx.\n") fmt.Fprintf(os.Stderr, "Run '%s --help' for usage.\n", executableBase) diff --git a/src/pkg/podman/container.go b/src/pkg/podman/container.go index bbc41a4e4..05c1d7d21 100644 --- a/src/pkg/podman/container.go +++ b/src/pkg/podman/container.go @@ -285,6 +285,14 @@ func (containers *Containers) Get() Container { return &container } +func (containers *Containers) Len() int { + if containers == nil { + return 0 + } + + return len(containers.data) +} + func (containers *Containers) Next() bool { available := containers.i < len(containers.data) if available {