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

…rors

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.

Fixes containers#937

src/cmd/create.go

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

src/cmd/enter.go

@@ -104,19 +104,23 @@ func enter(cmd *cobra.Command, args []string) error {
 		}
 	}
 
-	var release string
+	distro, err := utils.ParseDistro(enterFlags.distro)
+	if err != nil {
+		err := createErrorInvalidDistro()
+		return err
+	}
+
 	if enterFlags.release != "" {
 		defaultContainer = false
+    }
 
-		var err error
-		release, err = utils.ParseRelease(enterFlags.distro, enterFlags.release)
-		if err != nil {
-			err := createErrorInvalidRelease()
-			return err
-		}
-	}
+    release, err := utils.ParseRelease(distro, enterFlags.release)
+    if err != nil {
+        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

@@ -86,7 +86,7 @@ func run(cmd *cobra.Command, args []string) error {
 		return nil
 	}
 
-	var defaultContainer bool = true
+	var defaultContainer = true
 
 	if runFlags.container != "" {
 		defaultContainer = false
@@ -102,17 +102,21 @@ func run(cmd *cobra.Command, args []string) error {
 		}
 	}
 
-	var release string
+	distro, err := utils.ParseDistro(runFlags.distro)
+	if err != nil {
+		err := createErrorInvalidDistro()
+		return err
+	}
+
 	if runFlags.release != "" {
 		defaultContainer = false
+    }
 
-		var err error
-		release, err = utils.ParseRelease(runFlags.distro, runFlags.release)
-		if err != nil {
-			err := createErrorInvalidRelease()
-			return err
-		}
-	}
+    release, err := utils.ParseRelease(distro, runFlags.release)
+    if err != nil {
+        err := createErrorInvalidRelease(distro)
+        return err
+    }
 
 	if len(args) == 0 {
 		var builder strings.Builder
@@ -125,7 +129,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", 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 distro %s are in format %s", 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",
 			"ubi",
 			parseReleaseRHEL,
+			"<major.minor>",
 			"registry.access.redhat.com",
 			"ubi8",
 			false,
@@ -407,6 +414,16 @@ func GetMountOptions(target string) (string, error) {
 	return mountOptions, nil
 }
 
+// GetReleaseFormat returns the format string signifying supported release
+// version formats.
+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 +463,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 +480,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,24 +632,30 @@ func ShortID(id string) string {
 	return id
 }
 
-func ParseRelease(distro, release string) (string, error) {
+// ParseDistro ...
+func ParseDistro(distro string) (string, error) {
 	if distro == "" {
 		distro = distroDefault
 		if viper.IsSet("general.distro") {
 			distro = viper.GetString("general.distro")
 		}
 	}
 
-	if _, supportedDistro := supportedDistros[distro]; !supportedDistro {
-		distro = "fedora"
+	if !IsDistroSupported(distro) {
+		return "", ErrUnsupportedDistro
 	}
 
-	distroObj, supportedDistro := supportedDistros[distro]
-	if !supportedDistro {
-		panicMsg := fmt.Sprintf("failed to find %s in the list of supported distributions", distro)
-		panic(panicMsg)
+	return distro, nil
+}
+
+// ParseRelease ...
+func ParseRelease(distro, release string) (string, error) {
+	if !IsDistroSupported(distro) {
+		return "", ErrUnsupportedDistro
 	}
 
+	distroObj := supportedDistros[distro]
+
 	parseRelease := distroObj.ParseRelease
 	release, err := parseRelease(release)
 	return release, err

src/pkg/utils/utils_test.go

@@ -23,6 +23,30 @@ import (
 	"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) {
+        res := GetReleaseFormat(tc.distro)
+        assert.Equal(t, tc.expected, res)
+    }
+}
+
 func TestImageReferenceCanBeID(t *testing.T) {
 	testCases := []struct {
 		name string
@@ -74,6 +98,72 @@ 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) {
+        res := IsDistroSupported(tc.distro)
+        assert.Equal(t, tc.ok, res)
+    }
+}
+
+func TestParseDistro(t *testing.T) {
+    testCases := []struct {
+        name string
+        distro string
+        expected string
+        err bool
+    }{
+        {
+            "Default - no distro provided",
+            "",
+            distroDefault,
+            false,
+        },
+        {
+            "Fedora",
+            "fedora",
+            "fedora",
+            false,
+        },
+        {
+            "RHEL",
+            "rhel",
+            "rhel",
+            false,
+        },
+        {
+            "FooBar; wrong distro",
+            "foobar",
+            "",
+            true,
+        },
+    }
+
+    for _, tc := range(testCases) {
+        res, err := ParseDistro(tc.distro)
+        assert.Equal(t, tc.expected, res)
+        if tc.err {
+            assert.NotNil(t, err)
+        }
+    }
+}
+
 func TestParseRelease(t *testing.T) {
 	testCases := []struct {
 		name         string
@@ -147,6 +237,12 @@ func TestParseRelease(t *testing.T) {
 			ok:           false,
 			errMsg:       "release must be a positive number",
 		},
+        {
+            name: "FooBar; unsupported distro",
+            inputDistro: "foobar",
+            ok: false,
+            err: ErrUnsupportedDistro,
+        },
 	}
 
 	for _, tc := range testCases {