containers
/
toolbox
mirror of https://github.com/containers/toolbox.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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:
  * 5c8ad7a7ec
  * 02f45fd3f2
  * 8b6418d8aa

... but retains the test cases that were not tied to the changes in
those commits.

[1] Commit fd756089ef
    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
https://github.com/containers/toolbox/pull/1080
This commit is contained in:
Debarshi Ray 2022-07-31 10:46:33 +02:00
parent 4f78c5ef86
commit 0e66af91fe
9 changed files with 34 additions and 282 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
src/cmd/create.go
View File

@ -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
}

16
src/cmd/enter.go
View File

@ -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
}

16
src/cmd/run.go
View File

@ -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
}

13
src/cmd/utils.go
View File

@ -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()

79
src/pkg/utils/utils.go
View File

@ -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,
"<release>/f<release>",
"registry.fedoraproject.org",
"",
false,
@ -112,7 +106,6 @@ var (
"rhel-toolbox",
"toolbox",
parseReleaseRHEL,
"<major.minor>",
"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") {

122
src/pkg/utils/utils_test.go
View File

@ -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

18
test/system/101-create.bats
View File

@ -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: <release>/f<release>"
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" {

18
test/system/104-run.bats
View File

@ -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: <release>/f<release>"
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" {

18
test/system/105-enter.bats
View File

@ -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: <release>/f<release>"
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" {