From 235e2d22ce270f5f724637f8c0fd74389365a86c Mon Sep 17 00:00:00 2001 From: Debarshi Ray Date: Sun, 31 Jul 2022 10:46:33 +0200 Subject: [PATCH] Revert "cmd, pkg/utils: Split distro and release parsing and ..." The idea of splitting ResolveContainerAndImageNames into two public functions [1] didn't turn out to be so useful [2]. Splitting things even further might make it worse. A better approach might be to (re-)unify the code further. This is the first step towards that. This reverts the following commits: * 5c8ad7a7ec56573bd17b80cc10d9e022ec313ac9 * 02f45fd3f2d5b545695fa7fa0cdf600ab9ba8a4e * 8b6418d8aa6b1573a413e084e83bd1f695475406 ... but retains the test cases that were not tied to the changes in those commits. [1] Commit fd756089efb89f05 https://github.com/containers/toolbox/pull/828 https://github.com/containers/toolbox/pull/838 [2] https://github.com/containers/toolbox/pull/977 https://github.com/containers/toolbox/issues/937 --- src/cmd/create.go | 16 ++--- src/cmd/enter.go | 16 ++--- src/cmd/run.go | 16 ++--- src/cmd/utils.go | 13 +--- src/pkg/utils/utils.go | 79 ++++------------------- src/pkg/utils/utils_test.go | 122 ------------------------------------ test/system/101-create.bats | 18 +----- test/system/104-run.bats | 18 +----- test/system/105-enter.bats | 18 +----- 9 files changed, 34 insertions(+), 282 deletions(-) diff --git a/src/cmd/create.go b/src/cmd/create.go index 41560cebe..4b203b099 100644 --- a/src/cmd/create.go +++ b/src/cmd/create.go @@ -154,23 +154,17 @@ func create(cmd *cobra.Command, args []string) error { } } - distro, err := utils.ResolveDistro(createFlags.distro) - if err != nil { - err := createErrorInvalidDistro() - return err - } - - release := createFlags.release - if release != "" { + var release string + if createFlags.release != "" { var err error - release, err = utils.ParseRelease(distro, release) + release, err = utils.ParseRelease(createFlags.distro, createFlags.release) if err != nil { - err := createErrorInvalidRelease(distro) + err := createErrorInvalidRelease() return err } } - image, release, err := utils.ResolveImageName(distro, createFlags.image, release) + image, release, err := utils.ResolveImageName(createFlags.distro, createFlags.image, release) if err != nil { return err } diff --git a/src/cmd/enter.go b/src/cmd/enter.go index 45b64f8ed..2a552edb8 100644 --- a/src/cmd/enter.go +++ b/src/cmd/enter.go @@ -107,25 +107,19 @@ func enter(cmd *cobra.Command, args []string) error { } } - distro, err := utils.ResolveDistro(enterFlags.distro) - if err != nil { - err := createErrorInvalidDistro() - return err - } - - release := enterFlags.release - if release != "" { + var release string + if enterFlags.release != "" { defaultContainer = false var err error - release, err = utils.ParseRelease(distro, release) + release, err = utils.ParseRelease(enterFlags.distro, enterFlags.release) if err != nil { - err := createErrorInvalidRelease(distro) + err := createErrorInvalidRelease() return err } } - image, release, err := utils.ResolveImageName(distro, "", release) + image, release, err := utils.ResolveImageName(enterFlags.distro, "", release) if err != nil { return err } diff --git a/src/cmd/run.go b/src/cmd/run.go index 91b21c49e..19bca34a1 100644 --- a/src/cmd/run.go +++ b/src/cmd/run.go @@ -107,20 +107,14 @@ func run(cmd *cobra.Command, args []string) error { } } - distro, err := utils.ResolveDistro(runFlags.distro) - if err != nil { - err := createErrorInvalidDistro() - return err - } - - release := runFlags.release - if release != "" { + var release string + if runFlags.release != "" { defaultContainer = false var err error - release, err = utils.ParseRelease(distro, release) + release, err = utils.ParseRelease(runFlags.distro, runFlags.release) if err != nil { - err := createErrorInvalidRelease(distro) + err := createErrorInvalidRelease() return err } } @@ -136,7 +130,7 @@ func run(cmd *cobra.Command, args []string) error { command := args - image, release, err := utils.ResolveImageName(distro, "", release) + image, release, err := utils.ResolveImageName(runFlags.distro, "", release) if err != nil { return err } diff --git a/src/cmd/utils.go b/src/cmd/utils.go index 8a288d579..3dd663c8e 100644 --- a/src/cmd/utils.go +++ b/src/cmd/utils.go @@ -81,20 +81,9 @@ func createErrorInvalidContainer(containerArg string) error { return errors.New(errMsg) } -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 { +func createErrorInvalidRelease() 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() diff --git a/src/pkg/utils/utils.go b/src/pkg/utils/utils.go index 28a27e0be..cba9149d0 100644 --- a/src/pkg/utils/utils.go +++ b/src/pkg/utils/utils.go @@ -44,7 +44,6 @@ type Distro struct { ContainerNamePrefix string ImageBasename string ParseRelease ParseReleaseFunc - ReleaseFormat string Registry string Repository string RepositoryNeedsRelease bool @@ -61,10 +60,6 @@ const ( ContainerNameRegexp = "[a-zA-Z0-9][a-zA-Z0-9_.-]*" ) -var ( - ErrUnsupportedDistro = errors.New("linux distribution is not supported") -) - var ( containerNamePrefixDefault = "fedora-toolbox" @@ -103,7 +98,6 @@ var ( "fedora-toolbox", "fedora-toolbox", parseReleaseFedora, - "/f", "registry.fedoraproject.org", "", false, @@ -112,7 +106,6 @@ var ( "rhel-toolbox", "toolbox", parseReleaseRHEL, - "", "registry.access.redhat.com", "ubi8", false, @@ -253,8 +246,8 @@ func getDefaultImageForDistro(distro, release string) string { panic("distro not specified") } - if !IsDistroSupported(distro) { - distro = distroDefault + if _, supportedDistro := supportedDistros[distro]; !supportedDistro { + distro = "fedora" } distroObj, supportedDistro := supportedDistros[distro] @@ -418,20 +411,6 @@ 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 { @@ -488,14 +467,6 @@ 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) @@ -640,18 +611,16 @@ 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, _ = ResolveDistro(distro) + distro = distroDefault + if viper.IsSet("general.distro") { + distro = viper.GetString("general.distro") + } } - if !IsDistroSupported(distro) { - distro = distroDefault + if _, supportedDistro := supportedDistros[distro]; !supportedDistro { + distro = "fedora" } distroObj, supportedDistro := supportedDistros[distro] @@ -756,33 +725,6 @@ 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). @@ -797,7 +739,10 @@ func ResolveImageName(distroCLI, imageCLI, releaseCLI string) (string, string, e distro, image, release := distroCLI, imageCLI, releaseCLI if distroCLI == "" { - distro, _ = ResolveDistro(distroCLI) + distro = distroDefault + if viper.IsSet("general.distro") { + distro = viper.GetString("general.distro") + } } if distro != distroDefault && releaseCLI == "" && !viper.IsSet("general.release") { diff --git a/src/pkg/utils/utils_test.go b/src/pkg/utils/utils_test.go index 2a5e45a14..329228154 100644 --- a/src/pkg/utils/utils_test.go +++ b/src/pkg/utils/utils_test.go @@ -20,45 +20,9 @@ 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 @@ -110,92 +74,6 @@ 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 diff --git a/test/system/101-create.bats b/test/system/101-create.bats index 396415fa9..d52f02808 100644 --- a/test/system/101-create.bats +++ b/test/system/101-create.bats @@ -80,27 +80,13 @@ teardown() { assert_line --index 2 "Use 'toolbox --verbose ...' for further details." } -@test "create: Try to create a container based on unsupported distribution" { - local distro="foo" - - run $TOOLBOX -y create -d "$distro" - - assert_failure - assert_line --index 0 "Error: invalid argument for '--distro'" - # Distro names are in a hashtable and thus the order can change - assert_line --index 1 --regexp "Supported values are: (.?(fedora|rhel))+" - assert_line --index 2 "Run 'toolbox --help' for usage." - assert [ ${#lines[@]} -eq 3 ] -} - @test "create: Try to create a container based on Fedora but with wrong version" { run $TOOLBOX -y create -d fedora -r foobar assert_failure assert_line --index 0 "Error: invalid argument for '--release'" - assert_line --index 1 "Supported values for distribution fedora are in format: /f" - assert_line --index 2 "Run 'toolbox --help' for usage." - assert [ ${#lines[@]} -eq 3 ] + assert_line --index 1 "Run 'toolbox --help' for usage." + assert [ ${#lines[@]} -eq 2 ] } @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 index 5bc8d33fd..67c9b8a3b 100644 --- a/test/system/104-run.bats +++ b/test/system/104-run.bats @@ -48,27 +48,13 @@ teardown() { assert_line --index 2 "Run 'toolbox --help' for usage." } -@test "run: Try to run a command in a container based on unsupported distribution" { - local distro="foo" - - run $TOOLBOX -y run -d "$distro" ls - - assert_failure - assert_line --index 0 "Error: invalid argument for '--distro'" - # Distro names are in a hashtable and thus the order can change - assert_line --index 1 --regexp "Supported values are: (.?(fedora|rhel))+" - 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" { run $TOOLBOX run -d fedora -r foobar ls assert_failure assert_line --index 0 "Error: invalid argument for '--release'" - assert_line --index 1 "Supported values for distribution fedora are in format: /f" - assert_line --index 2 "Run 'toolbox --help' for usage." - assert [ ${#lines[@]} -eq 3 ] + assert_line --index 1 "Run 'toolbox --help' for usage." + assert [ ${#lines[@]} -eq 2 ] } @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 index ec255ca5c..d593ca0d1 100644 --- a/test/system/105-enter.bats +++ b/test/system/105-enter.bats @@ -54,27 +54,13 @@ teardown() { assert_line --index 2 "Run 'toolbox --help' for usage." } -@test "enter: Try to enter a container based on unsupported distribution" { - local distro="foo" - - run $TOOLBOX -y enter -d "$distro" - - assert_failure - assert_line --index 0 "Error: invalid argument for '--distro'" - # Distro names are in a hashtable and thus the order can change - assert_line --index 1 --regexp "Supported values are: (.?(fedora|rhel))+" - 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" { run $TOOLBOX enter -d fedora -r foobar assert_failure assert_line --index 0 "Error: invalid argument for '--release'" - assert_line --index 1 "Supported values for distribution fedora are in format: /f" - assert_line --index 2 "Run 'toolbox --help' for usage." - assert [ ${#lines[@]} -eq 3 ] + assert_line --index 1 "Run 'toolbox --help' for usage." + assert [ ${#lines[@]} -eq 2 ] } @test "enter: Try to enter a container based on non-default distro without providing a version" {