From a6b4191d54ab7b7d0e930293f26ab43896d97afb Mon Sep 17 00:00:00 2001 From: Debarshi Ray Date: Fri, 29 Jul 2022 20:39:31 +0200 Subject: [PATCH] pkg/utils: Re-unify container & image name resolution The idea of splitting ResolveContainerAndImageNames into two public functions [1] didn't turn out to be so useful [2]. It pushed the burden on the callers, who needed to carefully call the split functions in the right order, because the container, distro, image and release values are very tightly related. This opens the door for mistakes. A better approach would be to restore ResolveContainerAndImageNames as the single public API. If necessary it could be internally split into smaller private functions. It would keep things simple for the callers. Note that this commit doesn't include the private split. If necessary, it can be done in future. This reverts commit fd756089efb89f05cd8c163606659f21c7c45cd8. [1] Commit fd756089efb89f05 https://github.com/containers/toolbox/pull/828 https://github.com/containers/toolbox/pull/838 [2] https://github.com/containers/toolbox/pull/977 https://github.com/containers/toolbox/issues/937 --- src/cmd/create.go | 10 +++--- src/cmd/enter.go | 7 +--- src/cmd/rootMigrationPath.go | 7 +--- src/cmd/run.go | 7 +--- src/pkg/utils/utils.go | 64 +++++++++++++----------------------- 5 files changed, 30 insertions(+), 65 deletions(-) diff --git a/src/cmd/create.go b/src/cmd/create.go index 4b203b099..f2a33e1b4 100644 --- a/src/cmd/create.go +++ b/src/cmd/create.go @@ -164,12 +164,10 @@ func create(cmd *cobra.Command, args []string) error { } } - image, release, err := utils.ResolveImageName(createFlags.distro, createFlags.image, release) - if err != nil { - return err - } - - container, err = utils.ResolveContainerName(container, image, release) + container, image, release, err := utils.ResolveContainerAndImageNames(container, + createFlags.distro, + createFlags.image, + release) if err != nil { return err } diff --git a/src/cmd/enter.go b/src/cmd/enter.go index 2a552edb8..1f0471779 100644 --- a/src/cmd/enter.go +++ b/src/cmd/enter.go @@ -119,12 +119,7 @@ func enter(cmd *cobra.Command, args []string) error { } } - image, release, err := utils.ResolveImageName(enterFlags.distro, "", release) - if err != nil { - return err - } - - container, err = utils.ResolveContainerName(container, image, release) + container, image, release, err := utils.ResolveContainerAndImageNames(container, enterFlags.distro, "", release) if err != nil { return err } diff --git a/src/cmd/rootMigrationPath.go b/src/cmd/rootMigrationPath.go index 0c5d6e253..f3b9f4582 100644 --- a/src/cmd/rootMigrationPath.go +++ b/src/cmd/rootMigrationPath.go @@ -60,12 +60,7 @@ func rootRunImpl(cmd *cobra.Command, args []string) error { return nil } - image, release, err := utils.ResolveImageName("", "", "") - if err != nil { - return err - } - - container, err := utils.ResolveContainerName("", image, release) + container, image, release, err := utils.ResolveContainerAndImageNames("", "", "", "") if err != nil { return err } diff --git a/src/cmd/run.go b/src/cmd/run.go index 19bca34a1..106c867fe 100644 --- a/src/cmd/run.go +++ b/src/cmd/run.go @@ -130,12 +130,7 @@ func run(cmd *cobra.Command, args []string) error { command := args - image, release, err := utils.ResolveImageName(runFlags.distro, "", release) - if err != nil { - return err - } - - container, err := utils.ResolveContainerName(runFlags.container, image, release) + container, image, release, err := utils.ResolveContainerAndImageNames(runFlags.container, runFlags.distro, "", release) if err != nil { return err } diff --git a/src/pkg/utils/utils.go b/src/pkg/utils/utils.go index 60cbac04f..1ad9ef590 100644 --- a/src/pkg/utils/utils.go +++ b/src/pkg/utils/utils.go @@ -582,13 +582,7 @@ func SetUpConfiguration() error { } } - image, release, err := ResolveImageName("", "", "") - if err != nil { - logrus.Debugf("Setting up configuration: failed to resolve image name: %s", err) - return errors.New("failed to resolve image name") - } - - container, err := ResolveContainerName("", image, release) + container, _, _, err := ResolveContainerAndImageNames("", "", "", "") if err != nil { logrus.Debugf("Setting up configuration: failed to resolve container name: %s", err) return errors.New("failed to resolve container name") @@ -693,41 +687,15 @@ func IsInsideToolboxContainer() bool { return PathExists("/run/.toolboxenv") } -// ResolveContainerName standardizes the name of a container -// -// If no container name is specified then the name of the image will be used. -func ResolveContainerName(container, image, release string) (string, error) { - logrus.Debug("Resolving container name") - logrus.Debugf("Container: '%s'", container) - logrus.Debugf("Image: '%s'", image) - logrus.Debugf("Release: '%s'", release) - - if container == "" { - var err error - container, err = GetContainerNamePrefixForImage(image) - if err != nil { - return "", err - } - - tag := ImageReferenceGetTag(image) - if tag != "" { - container = container + "-" + tag - } - } - - logrus.Debug("Resolved container name") - logrus.Debugf("Container: '%s'", container) - - return container, nil -} - -// ResolveImageName standardizes the name of an image. +// ResolveContainerAndImageNames takes care of standardizing names of containers and images. // // If no image name is specified then the base image will reflect the platform of the host (even the version). +// If no container name is specified then the name of the image will be used. // // If the host system is unknown then the base image will be 'fedora-toolbox' with a default version -func ResolveImageName(distroCLI, imageCLI, releaseCLI string) (string, string, error) { - logrus.Debug("Resolving image name") +func ResolveContainerAndImageNames(container, distroCLI, imageCLI, releaseCLI string) (string, string, string, error) { + logrus.Debug("Resolving container and image names") + logrus.Debugf("Container: '%s'", container) logrus.Debugf("Distribution (CLI): '%s'", distroCLI) logrus.Debugf("Image (CLI): '%s'", imageCLI) logrus.Debugf("Release (CLI): '%s'", releaseCLI) @@ -742,7 +710,7 @@ func ResolveImageName(distroCLI, imageCLI, releaseCLI string) (string, string, e } if distro != distroDefault && releaseCLI == "" && !viper.IsSet("general.release") { - return "", "", fmt.Errorf("release not found for non-default distribution %s", distro) + return "", "", "", fmt.Errorf("release not found for non-default distribution %s", distro) } if releaseCLI == "" { @@ -776,9 +744,23 @@ func ResolveImageName(distroCLI, imageCLI, releaseCLI string) (string, string, e } } - logrus.Debug("Resolved image name") + if container == "" { + var err error + container, err = GetContainerNamePrefixForImage(image) + if err != nil { + return "", "", "", err + } + + tag := ImageReferenceGetTag(image) + if tag != "" { + container = container + "-" + tag + } + } + + logrus.Debug("Resolved container and image names") + logrus.Debugf("Container: '%s'", container) logrus.Debugf("Image: '%s'", image) logrus.Debugf("Release: '%s'", release) - return image, release, nil + return container, image, release, nil }