From 8454b31a823f7f5aaaeb26643b38a0d1a46e52b2 Mon Sep 17 00:00:00 2001 From: Debarshi Ray Date: Mon, 1 Aug 2022 23:44:49 +0200 Subject: [PATCH] cmd/utils, pkg/utils: Improve the error messages for the distro option https://github.com/containers/toolbox/issues/937 https://github.com/containers/toolbox/pull/1103 --- src/cmd/utils.go | 32 ++++++++++++++++++++++++++++++++ src/pkg/utils/errors.go | 14 ++++++++++++++ src/pkg/utils/utils.go | 8 ++++++-- test/system/101-create.bats | 12 ++++++++---- test/system/104-run.bats | 12 ++++++++---- test/system/105-enter.bats | 12 ++++++++---- 6 files changed, 76 insertions(+), 14 deletions(-) diff --git a/src/cmd/utils.go b/src/cmd/utils.go index 911d0a7..8265e55 100644 --- a/src/cmd/utils.go +++ b/src/cmd/utils.go @@ -71,6 +71,16 @@ func createErrorContainerNotFound(container string) error { return errors.New(errMsg) } +func createErrorDistroWithoutRelease(distro string) error { + var builder strings.Builder + fmt.Fprintf(&builder, "option '--release' is needed\n") + fmt.Fprintf(&builder, "Distribution %s doesn't match the host.\n", distro) + fmt.Fprintf(&builder, "Run '%s --help' for usage.", executableBase) + + errMsg := builder.String() + return errors.New(errMsg) +} + func createErrorInvalidContainer(containerArg string) error { var builder strings.Builder fmt.Fprintf(&builder, "invalid argument for '%s'\n", containerArg) @@ -81,6 +91,16 @@ func createErrorInvalidContainer(containerArg string) error { return errors.New(errMsg) } +func createErrorInvalidDistro(distro string) error { + var builder strings.Builder + fmt.Fprintf(&builder, "invalid argument for '--distro'\n") + fmt.Fprintf(&builder, "Distribution %s is unsupported.\n", distro) + fmt.Fprintf(&builder, "Run '%s --help' for usage.", executableBase) + + errMsg := builder.String() + return errors.New(errMsg) +} + func createErrorInvalidImageForContainerName(container string) error { var builder strings.Builder fmt.Fprintf(&builder, "invalid argument for '--image'\n") @@ -122,6 +142,7 @@ func resolveContainerAndImageNames(container, containerArg, distroCLI, imageCLI, if err != nil { var errContainer *utils.ContainerError + var errDistro *utils.DistroError var errParseRelease *utils.ParseReleaseError if errors.As(err, &errContainer) { @@ -140,6 +161,17 @@ func resolveContainerAndImageNames(container, containerArg, distroCLI, imageCLI, panicMsg := fmt.Sprintf("unexpected %T: %s", err, err) panic(panicMsg) } + } else if errors.As(err, &errDistro) { + if errors.Is(err, utils.ErrDistroUnsupported) { + err := createErrorInvalidDistro(errDistro.Distro) + return "", "", "", err + } else if errors.Is(err, utils.ErrDistroWithoutRelease) { + err := createErrorDistroWithoutRelease(errDistro.Distro) + return "", "", "", err + } else { + panicMsg := fmt.Sprintf("unexpected %T: %s", err, err) + panic(panicMsg) + } } else if errors.As(err, &errParseRelease) { err := createErrorInvalidRelease(errParseRelease.Hint) return "", "", "", err diff --git a/src/pkg/utils/errors.go b/src/pkg/utils/errors.go index 754e5a1..0b7adf5 100644 --- a/src/pkg/utils/errors.go +++ b/src/pkg/utils/errors.go @@ -26,6 +26,11 @@ type ContainerError struct { Err error } +type DistroError struct { + Distro string + Err error +} + type ParseReleaseError struct { Hint string } @@ -39,6 +44,15 @@ func (err *ContainerError) Unwrap() error { return err.Err } +func (err *DistroError) Error() string { + errMsg := fmt.Sprintf("%s: %s", err.Distro, err.Err) + return errMsg +} + +func (err *DistroError) Unwrap() error { + return err.Err +} + func (err *ParseReleaseError) Error() string { return err.Hint } diff --git a/src/pkg/utils/utils.go b/src/pkg/utils/utils.go index 0994a95..83ea8e7 100644 --- a/src/pkg/utils/utils.go +++ b/src/pkg/utils/utils.go @@ -121,6 +121,10 @@ var ( ErrContainerNameFromImageInvalid = errors.New("container name generated from image is invalid") ErrContainerNameInvalid = errors.New("container name is invalid") + + ErrDistroUnsupported = errors.New("distribution is unsupported") + + ErrDistroWithoutRelease = errors.New("non-default distribution must specify release") ) func init() { @@ -713,11 +717,11 @@ func ResolveContainerAndImageNames(container, distroCLI, imageCLI, releaseCLI st } if _, ok := supportedDistros[distro]; !ok { - return "", "", "", fmt.Errorf("distribution %s is unsupported", distro) + return "", "", "", &DistroError{distro, ErrDistroUnsupported} } if distro != distroDefault && releaseCLI == "" && !viper.IsSet("general.release") { - return "", "", "", fmt.Errorf("release not found for non-default distribution %s", distro) + return "", "", "", &DistroError{distro, ErrDistroWithoutRelease} } if releaseCLI == "" { diff --git a/test/system/101-create.bats b/test/system/101-create.bats index e03518c..6b09ff9 100644 --- a/test/system/101-create.bats +++ b/test/system/101-create.bats @@ -90,8 +90,10 @@ teardown() { run $TOOLBOX --assumeyes create --distro "$distro" assert_failure - assert_line --index 0 "Error: distribution $distro is unsupported" - assert [ ${#lines[@]} -eq 1 ] + assert_line --index 0 "Error: invalid argument for '--distro'" + assert_line --index 1 "Distribution $distro is unsupported." + assert_line --index 2 "Run 'toolbox --help' for usage." + assert [ ${#lines[@]} -eq 3 ] } @test "create: Try to create a container based on non-existent image" { @@ -158,8 +160,10 @@ teardown() { run $TOOLBOX -y create -d "$distro" assert_failure - assert_line --index 0 "Error: release not found for non-default distribution $distro" - assert [ ${#lines[@]} -eq 1 ] + assert_line --index 0 "Error: option '--release' is needed" + assert_line --index 1 "Distribution $distro doesn't match the host." + assert_line --index 2 "Run 'toolbox --help' for usage." + assert [ ${#lines[@]} -eq 3 ] } @test "create: Try to create a container and pass a non-existent file to the --authfile option" { diff --git a/test/system/104-run.bats b/test/system/104-run.bats index 562a68f..5bb92f8 100644 --- a/test/system/104-run.bats +++ b/test/system/104-run.bats @@ -54,8 +54,10 @@ teardown() { run $TOOLBOX --assumeyes run --distro "$distro" ls assert_failure - assert_line --index 0 "Error: distribution $distro is unsupported" - assert [ ${#lines[@]} -eq 1 ] + assert_line --index 0 "Error: invalid argument for '--distro'" + assert_line --index 1 "Distribution $distro is unsupported." + 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 Fedora but with wrong version" { @@ -113,8 +115,10 @@ teardown() { run $TOOLBOX run -d "$distro" ls assert_failure - assert_line --index 0 "Error: release not found for non-default distribution $distro" - assert [ ${#lines[@]} -eq 1 ] + assert_line --index 0 "Error: option '--release' is needed" + assert_line --index 1 "Distribution $distro doesn't match the host." + assert_line --index 2 "Run 'toolbox --help' for usage." + assert [ ${#lines[@]} -eq 3 ] } @test "run: Run echo 'Hello World' inside of the default container" { diff --git a/test/system/105-enter.bats b/test/system/105-enter.bats index 8079800..788f989 100644 --- a/test/system/105-enter.bats +++ b/test/system/105-enter.bats @@ -60,8 +60,10 @@ teardown() { run $TOOLBOX --assumeyes enter --distro "$distro" assert_failure - assert_line --index 0 "Error: distribution $distro is unsupported" - assert [ ${#lines[@]} -eq 1 ] + assert_line --index 0 "Error: invalid argument for '--distro'" + assert_line --index 1 "Distribution $distro is unsupported." + assert_line --index 2 "Run 'toolbox --help' for usage." + assert [ ${#lines[@]} -eq 3 ] } @test "enter: Try to enter a container based on Fedora but with wrong version" { @@ -119,8 +121,10 @@ teardown() { run $TOOLBOX enter -d "$distro" assert_failure - assert_line --index 0 "Error: release not found for non-default distribution $distro" - assert [ ${#lines[@]} -eq 1 ] + assert_line --index 0 "Error: option '--release' is needed" + assert_line --index 1 "Distribution $distro doesn't match the host." + assert_line --index 2 "Run 'toolbox --help' for usage." + assert [ ${#lines[@]} -eq 3 ] } # TODO: Write the test