cmd, pkg/utils: Split distro and release parsing and report better errors

Using a non-supported distribution via `--distro` resulted in a silent
fallback to the Fedora image which confuses users. When a faulty release
format was used with `--release` a message without any hint about the
correct format was shown to the user.

This separates distro and release parsing into two chunks that have
greater control over error reporting and provides more accurate error
reports for the user.

Fixes #937

#977

Author: martymichal

src/cmd/create.go

@@ -138,17 +138,23 @@ func create(cmd *cobra.Command, args []string) error {
 		}
 	}
 
-	var release string
-	if createFlags.release != "" {
+	distro, err := utils.ResolveDistro(createFlags.distro)
+	if err != nil {
+		err := createErrorInvalidDistro()
+		return err
+	}
+
+	release := createFlags.release
+	if release != "" {
 		var err error
-		release, err = utils.ParseRelease(createFlags.distro, createFlags.release)
+		release, err = utils.ParseRelease(distro, release)
 		if err != nil {
-			err := createErrorInvalidRelease()
+			err := createErrorInvalidRelease(distro)
 			return err
 		}
 	}
 
-	image, release, err := utils.ResolveImageName(createFlags.distro, createFlags.image, release)
+	image, release, err := utils.ResolveImageName(distro, createFlags.image, release)
 	if err != nil {
 		return err
 	}

src/cmd/enter.go

@@ -104,19 +104,25 @@ func enter(cmd *cobra.Command, args []string) error {
 		}
 	}
 
-	var release string
-	if enterFlags.release != "" {
+	distro, err := utils.ResolveDistro(enterFlags.distro)
+	if err != nil {
+		err := createErrorInvalidDistro()
+		return err
+	}
+
+	release := enterFlags.release
+	if release != "" {
 		defaultContainer = false
 
 		var err error
-		release, err = utils.ParseRelease(enterFlags.distro, enterFlags.release)
+		release, err = utils.ParseRelease(distro, release)
 		if err != nil {
-			err := createErrorInvalidRelease()
+			err := createErrorInvalidRelease(distro)
 			return err
 		}
 	}
 
-	image, release, err := utils.ResolveImageName(enterFlags.distro, "", release)
+	image, release, err := utils.ResolveImageName(distro, "", release)
 	if err != nil {
 		return err
 	}

src/cmd/run.go

@@ -102,14 +102,20 @@ func run(cmd *cobra.Command, args []string) error {
 		}
 	}
 
-	var release string
-	if runFlags.release != "" {
+	distro, err := utils.ResolveDistro(runFlags.distro)
+	if err != nil {
+		err := createErrorInvalidDistro()
+		return err
+	}
+
+	release := runFlags.release
+	if release != "" {
 		defaultContainer = false
 
 		var err error
-		release, err = utils.ParseRelease(runFlags.distro, runFlags.release)
+		release, err = utils.ParseRelease(distro, release)
 		if err != nil {
-			err := createErrorInvalidRelease()
+			err := createErrorInvalidRelease(distro)
 			return err
 		}
 	}
@@ -125,7 +131,7 @@ func run(cmd *cobra.Command, args []string) error {
 
 	command := args
 
-	image, release, err := utils.ResolveImageName(runFlags.distro, "", release)
+	image, release, err := utils.ResolveImageName(distro, "", release)
 	if err != nil {
 		return err
 	}

src/cmd/utils.go

@@ -23,6 +23,8 @@ import (
 	"os/exec"
 	"strings"
 	"syscall"
+
+	"github.com/containers/toolbox/pkg/utils"
 )
 
 // askForConfirmation prints prompt to stdout and waits for response from the
@@ -69,9 +71,20 @@ func createErrorContainerNotFound(container string) error {
 	return errors.New(errMsg)
 }
 
-func createErrorInvalidRelease() error {
+func createErrorInvalidDistro() error {
+	var builder strings.Builder
+	fmt.Fprintf(&builder, "invalid argument for '--distro'\n")
+	fmt.Fprintf(&builder, "Supported values are: %s\n", strings.Join(utils.GetSupportedDistros(), " "))
+	fmt.Fprintf(&builder, "Run '%s --help' for usage.", executableBase)
+
+	errMsg := builder.String()
+	return errors.New(errMsg)
+}
+
+func createErrorInvalidRelease(distro string) error {
 	var builder strings.Builder
 	fmt.Fprintf(&builder, "invalid argument for '--release'\n")
+	fmt.Fprintf(&builder, "Supported values for distribution %s are in format: %s\n", distro, utils.GetReleaseFormat(distro))
 	fmt.Fprintf(&builder, "Run '%s --help' for usage.", executableBase)
 
 	errMsg := builder.String()

src/pkg/utils/utils.go

@@ -44,6 +44,7 @@ type Distro struct {
 	ContainerNamePrefix    string
 	ImageBasename          string
 	ParseRelease           ParseReleaseFunc
+	ReleaseFormat          string
 	Registry               string
 	Repository             string
 	RepositoryNeedsRelease bool
@@ -60,6 +61,10 @@ const (
 	ContainerNameRegexp = "[a-zA-Z0-9][a-zA-Z0-9_.-]*"
 )
 
+var (
+	ErrUnsupportedDistro = errors.New("linux distribution is not supported")
+)
+
 var (
 	containerNamePrefixDefault = "fedora-toolbox"
 
@@ -98,6 +103,7 @@ var (
 			"fedora-toolbox",
 			"fedora-toolbox",
 			parseReleaseFedora,
+			"<release>/f<release>",
 			"registry.fedoraproject.org",
 			"",
 			false,
@@ -106,6 +112,7 @@ var (
 			"rhel-toolbox",
 			"toolbox",
 			parseReleaseRHEL,
+			"<major.minor>",
 			"registry.access.redhat.com",
 			"ubi8",
 			false,
@@ -407,6 +414,20 @@ func GetMountOptions(target string) (string, error) {
 	return mountOptions, nil
 }
 
+// GetReleaseFormat returns the format string signifying supported release
+// version formats.
+//
+// distro should be value found under ID in os-release.
+//
+// If distro is unsupported an empty string is returned.
+func GetReleaseFormat(distro string) string {
+	if !IsDistroSupported(distro) {
+		return ""
+	}
+
+	return supportedDistros[distro].ReleaseFormat
+}
+
 func GetRuntimeDirectory(targetUser *user.User) (string, error) {
 	gid, err := strconv.Atoi(targetUser.Gid)
 	if err != nil {
@@ -446,6 +467,15 @@ func GetRuntimeDirectory(targetUser *user.User) (string, error) {
 	return toolboxRuntimeDirectory, nil
 }
 
+// GetSupportedDistros returns a list of supported distributions
+func GetSupportedDistros() []string {
+	var distros []string
+	for d := range supportedDistros {
+		distros = append(distros, d)
+	}
+	return distros
+}
+
 // HumanDuration accepts a Unix time value and converts it into a human readable
 // string.
 //
@@ -454,6 +484,14 @@ func HumanDuration(duration int64) string {
 	return units.HumanDuration(time.Since(time.Unix(duration, 0))) + " ago"
 }
 
+// IsDistroSupported signifies if a distribution has a toolbx image for it.
+//
+// distro should be value found under ID in os-release.
+func IsDistroSupported(distro string) bool {
+	_, ok := supportedDistros[distro]
+	return ok
+}
+
 // ImageReferenceCanBeID checks if 'image' might be the ID of an image
 func ImageReferenceCanBeID(image string) bool {
 	matched, err := regexp.MatchString("^[a-f0-9]{6,64}$", image)
@@ -598,6 +636,11 @@ func ShortID(id string) string {
 	return id
 }
 
+// ParseRelease assesses if the requested version of a distribution is in
+// the correct format.
+//
+// If distro is an empty string, a default value (value under the
+// 'general.distro' key in a config file or 'fedora') is assumed.
 func ParseRelease(distro, release string) (string, error) {
 	if distro == "" {
 		distro = distroDefault
@@ -712,6 +755,33 @@ func ResolveContainerName(container, image, release string) (string, error) {
 	return container, nil
 }
 
+// ResolveDistro assess if the requested distribution is supported and provides
+// a default value if none is requested.
+//
+// If distro is empty, and the "general.distro" key in a config file is set,
+// the value is read from the config file. If the key is not set, the default
+// value ('fedora') is used instead.
+func ResolveDistro(distro string) (string, error) {
+	logrus.Debug("Resolving distribution")
+	logrus.Debugf("Distribution: %s", distro)
+
+	if distro == "" {
+		distro = distroDefault
+		if viper.IsSet("general.distro") {
+			distro = viper.GetString("general.distro")
+		}
+	}
+
+	if !IsDistroSupported(distro) {
+		return "", ErrUnsupportedDistro
+	}
+
+	logrus.Debug("Resolved distribution")
+	logrus.Debugf("Distribution: %s", distro)
+
+	return distro, nil
+}
+
 // ResolveImageName standardizes the name of an image.
 //
 // If no image name is specified then the base image will reflect the platform of the host (even the version).

src/pkg/utils/utils_test.go

@@ -20,9 +20,45 @@ import (
 	"strconv"
 	"testing"
 
+	"github.com/spf13/viper"
 	"github.com/stretchr/testify/assert"
 )
 
+func TestGetReleaseFormat(t *testing.T) {
+	testCases := []struct {
+		name     string
+		distro   string
+		expected string
+	}{
+		{
+			"Unknown distro",
+			"foobar",
+			"",
+		},
+		{
+			"Known distro (fedora)",
+			"fedora",
+			supportedDistros["fedora"].ReleaseFormat,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			res := GetReleaseFormat(tc.distro)
+			assert.Equal(t, tc.expected, res)
+		})
+	}
+}
+
+func TestGetSupportedDistros(t *testing.T) {
+	refDistros := []string{"fedora", "rhel"}
+
+	distros := GetSupportedDistros()
+	for _, d := range distros {
+		assert.Contains(t, refDistros, d)
+	}
+}
+
 func TestImageReferenceCanBeID(t *testing.T) {
 	testCases := []struct {
 		name string
@@ -74,6 +110,92 @@ func TestImageReferenceCanBeID(t *testing.T) {
 	}
 }
 
+func TestIsDistroSupport(t *testing.T) {
+	testCases := []struct {
+		name   string
+		distro string
+		ok     bool
+	}{
+		{
+			"Unsupported distro",
+			"foobar",
+			false,
+		},
+		{
+			"Supported distro (fedora)",
+			"fedora",
+			true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			res := IsDistroSupported(tc.distro)
+			assert.Equal(t, tc.ok, res)
+		})
+	}
+}
+
+func TestResolveDistro(t *testing.T) {
+	testCases := []struct {
+		name        string
+		distro      string
+		expected    string
+		configValue string
+		err         bool
+	}{
+		{
+			"Default - no distro provided; config unset",
+			"",
+			distroDefault,
+			"",
+			false,
+		},
+		{
+			"Default - no distro provided; config set",
+			"",
+			"rhel",
+			"rhel",
+			false,
+		},
+		{
+			"Fedora",
+			"fedora",
+			"fedora",
+			"",
+			false,
+		},
+		{
+			"RHEL",
+			"rhel",
+			"rhel",
+			"",
+			false,
+		},
+		{
+			"FooBar; wrong distro",
+			"foobar",
+			"",
+			"",
+			true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			if tc.configValue != "" {
+				viper.Set("general.distro", tc.configValue)
+			}
+
+			res, err := ResolveDistro(tc.distro)
+			assert.Equal(t, tc.expected, res)
+			if tc.err {
+				assert.NotNil(t, err)
+			}
+		})
+	}
+}
+
 func TestParseRelease(t *testing.T) {
 	testCases := []struct {
 		name         string