Skip to content

Commit

Permalink
pkg/utils: Re-unify container & image name resolution
Browse files Browse the repository at this point in the history
The idea of splitting ResolveContainerAndImageNames into two public
functions didn't turn out to be so useful [1].  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, but internally split it into smaller private
functions, if necessary.  It would keep things simple for both the
callers and the implementation.

Note that this commit doesn't include the private split, but leaves
that as a future exercise.

This reverts commit fd75608.

[1] containers#977
  • Loading branch information
debarshiray committed Jul 31, 2022
1 parent f73fa94 commit 361075c
Show file tree
Hide file tree
Showing 5 changed files with 30 additions and 65 deletions.
10 changes: 4 additions & 6 deletions src/cmd/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -169,12 +169,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
}
Expand Down
7 changes: 1 addition & 6 deletions src/cmd/enter.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,12 +125,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
}
Expand Down
7 changes: 1 addition & 6 deletions src/cmd/rootMigrationPath.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
7 changes: 1 addition & 6 deletions src/cmd/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,12 +135,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
}
Expand Down
64 changes: 23 additions & 41 deletions src/pkg/utils/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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)
Expand All @@ -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 == "" {
Expand Down Expand Up @@ -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
}

0 comments on commit 361075c

Please sign in to comment.