cmd, pkg/utils: Clarify the error message if the release is invalid

Currently, if --release has an invalid argument, the error message doesn't give any hints as to what's an acceptable value. This can be confusing. eg., is 36 a valid argument for Fedora? Or is it f36? Or is it F36? Is 'rawhide' accepted? containers#937
debarshiray · Sep 1, 2022 · 7f901a5 · 7f901a5
1 parent 97f24f8
commit 7f901a5
Show file tree

Hide file tree

Showing 9 changed files with 131 additions and 21 deletions.
diff --git a/src/cmd/create.go b/src/cmd/create.go
@@ -159,7 +159,8 @@ func create(cmd *cobra.Command, args []string) error {
 		var err error
 		release, err = utils.ParseRelease(createFlags.distro, createFlags.release)
 		if err != nil {
-			err := createErrorInvalidRelease()
+			hint := err.Error()
+			err := createErrorInvalidRelease(hint)
 			return err
 		}
 	}

diff --git a/src/cmd/enter.go b/src/cmd/enter.go
@@ -114,7 +114,8 @@ func enter(cmd *cobra.Command, args []string) error {
 		var err error
 		release, err = utils.ParseRelease(enterFlags.distro, enterFlags.release)
 		if err != nil {
-			err := createErrorInvalidRelease()
+			hint := err.Error()
+			err := createErrorInvalidRelease(hint)
 			return err
 		}
 	}

diff --git a/src/cmd/run.go b/src/cmd/run.go
@@ -114,7 +114,8 @@ func run(cmd *cobra.Command, args []string) error {
 		var err error
 		release, err = utils.ParseRelease(runFlags.distro, runFlags.release)
 		if err != nil {
-			err := createErrorInvalidRelease()
+			hint := err.Error()
+			err := createErrorInvalidRelease(hint)
 			return err
 		}
 	}

diff --git a/src/cmd/utils.go b/src/cmd/utils.go
@@ -81,9 +81,10 @@ func createErrorInvalidContainer(containerArg string) error {
 	return errors.New(errMsg)
 }
 
-func createErrorInvalidRelease() error {
+func createErrorInvalidRelease(hint string) error {
 	var builder strings.Builder
 	fmt.Fprintf(&builder, "invalid argument for '--release'\n")
+	fmt.Fprintf(&builder, "%s\n", hint)
 	fmt.Fprintf(&builder, "Run '%s --help' for usage.", executableBase)
 
 	errMsg := builder.String()

diff --git a/src/pkg/utils/utils.go b/src/pkg/utils/utils.go
@@ -635,28 +635,30 @@ func parseReleaseFedora(release string) (string, error) {
 
 	releaseN, err := strconv.Atoi(release)
 	if err != nil {
-		return "", err
+		logrus.Debugf("Parsing release %s as an integer failed: %s", release, err)
+		return "", errors.New("The release must be a positive integer.")
 	}
 
 	if releaseN <= 0 {
-		return "", errors.New("release must be a positive integer")
+		return "", errors.New("The release must be a positive integer.")
 	}
 
 	return release, nil
 }
 
 func parseReleaseRHEL(release string) (string, error) {
 	if i := strings.IndexRune(release, '.'); i == -1 {
-		return "", errors.New("release must have a '.'")
+		return "", errors.New("The release must be in the '<major>.<minor>' format.")
 	}
 
 	releaseN, err := strconv.ParseFloat(release, 32)
 	if err != nil {
-		return "", err
+		logrus.Debugf("Parsing release %s as a float failed: %s", release, err)
+		return "", errors.New("The release must be in the '<major>.<minor>' format.")
 	}
 
 	if releaseN <= 0 {
-		return "", errors.New("release must be a positive number")
+		return "", errors.New("The release must be a positive number.")
 	}
 
 	return release, nil

diff --git a/src/pkg/utils/utils_test.go b/src/pkg/utils/utils_test.go
@@ -17,7 +17,6 @@
 package utils
 
 import (
-	"strconv"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -103,14 +102,14 @@ func TestParseRelease(t *testing.T) {
 			inputDistro:  "fedora",
 			inputRelease: "-3",
 			ok:           false,
-			errMsg:       "release must be a positive integer",
+			errMsg:       "The release must be a positive integer.",
 		},
 		{
 			name:         "Fedora; foo; invalid; non-numeric",
 			inputDistro:  "fedora",
 			inputRelease: "foo",
 			ok:           false,
-			err:          strconv.ErrSyntax,
+			errMsg:       "The release must be a positive integer.",
 		},
 		{
 			name:         "RHEL; 8.3; valid",
@@ -131,21 +130,21 @@ func TestParseRelease(t *testing.T) {
 			inputDistro:  "rhel",
 			inputRelease: "8",
 			ok:           false,
-			errMsg:       "release must have a '.'",
+			errMsg:       "The release must be in the '<major>.<minor>' format.",
 		},
 		{
 			name:         "RHEL; 8.2foo; invalid; non-float",
 			inputDistro:  "rhel",
 			inputRelease: "8.2foo",
 			ok:           false,
-			err:          strconv.ErrSyntax,
+			errMsg:       "The release must be in the '<major>.<minor>' format.",
 		},
 		{
 			name:         "RHEL; -2.1; invalid; less than 0",
 			inputDistro:  "rhel",
 			inputRelease: "-2.1",
 			ok:           false,
-			errMsg:       "release must be a positive number",
+			errMsg:       "The release must be a positive number.",
 		},
 	}
 

diff --git a/test/system/101-create.bats b/test/system/101-create.bats
@@ -95,8 +95,43 @@ teardown() {
 
   assert_failure
   assert_line --index 0 "Error: invalid argument for '--release'"
-  assert_line --index 1 "Run 'toolbox --help' for usage."
-  assert [ ${#lines[@]} -eq 2 ]
+  assert_line --index 1 "The release must be a positive integer."
+  assert_line --index 2 "Run 'toolbox --help' for usage."
+  assert [ ${#lines[@]} -eq 3 ]
+
+  run $TOOLBOX --assumeyes create --distro fedora --release -3
+
+  assert_failure
+  assert_line --index 0 "Error: invalid argument for '--release'"
+  assert_line --index 1 "The release must be a positive integer."
+  assert_line --index 2 "Run 'toolbox --help' for usage."
+  assert [ ${#lines[@]} -eq 3 ]
+}
+
+@test "create: Try to create a container based on RHEL but with wrong version" {
+  run $TOOLBOX --assumeyes create --distro rhel --release 8
+
+  assert_failure
+  assert_line --index 0 "Error: invalid argument for '--release'"
+  assert_line --index 1 "The release must be in the '<major>.<minor>' format."
+  assert_line --index 2 "Run 'toolbox --help' for usage."
+  assert [ ${#lines[@]} -eq 3 ]
+
+  run $TOOLBOX --assumeyes create --distro rhel --release 8.2foo
+
+  assert_failure
+  assert_line --index 0 "Error: invalid argument for '--release'"
+  assert_line --index 1 "The release must be in the '<major>.<minor>' format."
+  assert_line --index 2 "Run 'toolbox --help' for usage."
+  assert [ ${#lines[@]} -eq 3 ]
+
+  run $TOOLBOX --assumeyes create --distro rhel --release -2.1
+
+  assert_failure
+  assert_line --index 0 "Error: invalid argument for '--release'"
+  assert_line --index 1 "The release must be a positive number."
+  assert_line --index 2 "Run 'toolbox --help' for usage."
+  assert [ ${#lines[@]} -eq 3 ]
 }
 
 @test "create: Try to create a container based on non-default distribution without providing version" {

diff --git a/test/system/104-run.bats b/test/system/104-run.bats
@@ -63,8 +63,43 @@ teardown() {
 
   assert_failure
   assert_line --index 0 "Error: invalid argument for '--release'"
-  assert_line --index 1 "Run 'toolbox --help' for usage."
-  assert [ ${#lines[@]} -eq 2 ]
+  assert_line --index 1 "The release must be a positive integer."
+  assert_line --index 2 "Run 'toolbox --help' for usage."
+  assert [ ${#lines[@]} -eq 3 ]
+
+  run $TOOLBOX run --distro fedora --release -3 ls
+
+  assert_failure
+  assert_line --index 0 "Error: invalid argument for '--release'"
+  assert_line --index 1 "The release must be a positive integer."
+  assert_line --index 2 "Run 'toolbox --help' for usage."
+  assert [ ${#lines[@]} -eq 3 ]
+}
+
+@test "run: Try to run a command in a container based on RHEL but with wrong version" {
+  run $TOOLBOX run --distro rhel --release 8 ls
+
+  assert_failure
+  assert_line --index 0 "Error: invalid argument for '--release'"
+  assert_line --index 1 "The release must be in the '<major>.<minor>' format."
+  assert_line --index 2 "Run 'toolbox --help' for usage."
+  assert [ ${#lines[@]} -eq 3 ]
+
+  run $TOOLBOX run --distro rhel --release 8.2foo ls
+
+  assert_failure
+  assert_line --index 0 "Error: invalid argument for '--release'"
+  assert_line --index 1 "The release must be in the '<major>.<minor>' format."
+  assert_line --index 2 "Run 'toolbox --help' for usage."
+  assert [ ${#lines[@]} -eq 3 ]
+
+  run $TOOLBOX run --distro rhel --release -2.1 ls
+
+  assert_failure
+  assert_line --index 0 "Error: invalid argument for '--release'"
+  assert_line --index 1 "The release must be a positive number."
+  assert_line --index 2 "Run 'toolbox --help' for usage."
+  assert [ ${#lines[@]} -eq 3 ]
 }
 
 @test "run: Try to run a command in a container based on non-default distro without providing a version" {

diff --git a/test/system/105-enter.bats b/test/system/105-enter.bats
@@ -69,8 +69,43 @@ teardown() {
 
   assert_failure
   assert_line --index 0 "Error: invalid argument for '--release'"
-  assert_line --index 1 "Run 'toolbox --help' for usage."
-  assert [ ${#lines[@]} -eq 2 ]
+  assert_line --index 1 "The release must be a positive integer."
+  assert_line --index 2 "Run 'toolbox --help' for usage."
+  assert [ ${#lines[@]} -eq 3 ]
+
+  run $TOOLBOX enter --distro fedora --release -3
+
+  assert_failure
+  assert_line --index 0 "Error: invalid argument for '--release'"
+  assert_line --index 1 "The release must be a positive integer."
+  assert_line --index 2 "Run 'toolbox --help' for usage."
+  assert [ ${#lines[@]} -eq 3 ]
+}
+
+@test "enter: Try to enter a container based on RHEL but with wrong version" {
+  run $TOOLBOX enter --distro rhel --release 8
+
+  assert_failure
+  assert_line --index 0 "Error: invalid argument for '--release'"
+  assert_line --index 1 "The release must be in the '<major>.<minor>' format."
+  assert_line --index 2 "Run 'toolbox --help' for usage."
+  assert [ ${#lines[@]} -eq 3 ]
+
+  run $TOOLBOX enter --distro rhel --release 8.2foo
+
+  assert_failure
+  assert_line --index 0 "Error: invalid argument for '--release'"
+  assert_line --index 1 "The release must be in the '<major>.<minor>' format."
+  assert_line --index 2 "Run 'toolbox --help' for usage."
+  assert [ ${#lines[@]} -eq 3 ]
+
+  run $TOOLBOX enter --distro rhel --release -2.1
+
+  assert_failure
+  assert_line --index 0 "Error: invalid argument for '--release'"
+  assert_line --index 1 "The release must be a positive number."
+  assert_line --index 2 "Run 'toolbox --help' for usage."
+  assert [ ${#lines[@]} -eq 3 ]
 }
 
 @test "enter: Try to enter a container based on non-default distro without providing a version" {