Repository: osbuild/osbuild-composer
Pull request #271 (2020-03-03 11:46:51)

osbuild-pipeline: validate distro when fetching it
We were verifying two things: if the passed distroArg exists in the
distribution mapping in common/types.go and if the it is an actually
registered distro. Since you cannot have distros registered that don't
correspond to a type, the first test is unnecessary.

Merge the two tests by moving the (much better) error message down into
the second test. This makes DistributionExists redundant, because
Registry.GetDistro() checks this implicitly.

Also, move ListDistributions() to the Registry object, because we want
to show distributions that are actually registered.

Add a test which checks that Registry.List() works and that all included
distributions register correctly.

larskarlitski: Rebased to include @ondrejbudai's linter fixes. Thanks!

Patch:
diff --git a/cmd/osbuild-pipeline/main.go b/cmd/osbuild-pipeline/main.go
index 4349e16d1..274d4f0f1 100644
--- a/cmd/osbuild-pipeline/main.go
+++ b/cmd/osbuild-pipeline/main.go
@@ -41,17 +41,6 @@ func main() {
 		return
 	}
 
-	// Validate distribution
-	if !common.DistributionExists(distroArg) {
-		_, _ = fmt.Fprintf(os.Stderr, "The provided distribution (%s) is not supported. Use one of these:\n", distroArg)
-		for _, distro := range common.ListDistributions() {
-			_, _ = fmt.Fprintln(os.Stderr, " *", distro)
-		}
-		return
-	}
-
-	// Validate image type
-
 	blueprint := &blueprint.Blueprint{}
 	if blueprintArg != "" {
 		file, err := ioutil.ReadFile(blueprintArg)
@@ -71,7 +60,11 @@ func main() {
 
 	d := distros.GetDistro(distroArg)
 	if d == nil {
-		panic("unknown distro: " + distroArg)
+		_, _ = fmt.Fprintf(os.Stderr, "The provided distribution (%s) is not supported. Use one of these:\n", distroArg)
+		for _, distro := range distros.List() {
+			_, _ = fmt.Fprintln(os.Stderr, " *", distro)
+		}
+		return
 	}
 
 	packages := make([]string, len(blueprint.Packages))
diff --git a/internal/common/types.go b/internal/common/types.go
index b517e9b36..88714299f 100644
--- a/internal/common/types.go
+++ b/internal/common/types.go
@@ -282,14 +282,6 @@ func (dist *Distribution) ModulePlatformID() (string, error) {
 	}
 }
 
-func ListDistributions() []string {
-	return listHelper(getDistributionMapping())
-}
-
-func DistributionExists(testedDistro string) bool {
-	return existsHelper(getDistributionMapping(), testedDistro)
-}
-
 func (distro *Distribution) UnmarshalJSON(data []byte) error {
 	value, err := unmarshalHelper(data, " is not a valid JSON value", " is not a valid distribution", getDistributionMapping())
 	if err != nil {
diff --git a/internal/distro/distro.go b/internal/distro/distro.go
index a9d14d686..2d49d49a7 100644
--- a/internal/distro/distro.go
+++ b/internal/distro/distro.go
@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"sort"
 	"strings"
 
 	"github.com/osbuild/osbuild-composer/internal/common"
@@ -123,6 +124,16 @@ func (r *Registry) GetDistro(name string) Distro {
 	return distro
 }
 
+// Returns the names of all distros in a Registry, sorted alphabetically.
+func (r *Registry) List() []string {
+	list := []string{}
+	for _, distro := range r.distros {
+		list = append(list, distro.Name())
+	}
+	sort.Strings(list)
+	return list
+}
+
 func (r *Registry) FromHost() (Distro, error) {
 	name, err := GetHostDistroName()
 	if err != nil {


Test patch:
diff --git a/internal/distro/distro_test.go b/internal/distro/distro_test.go
index b81d81992..72755d742 100644
--- a/internal/distro/distro_test.go
+++ b/internal/distro/distro_test.go
@@ -6,6 +6,8 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
+
 	"github.com/osbuild/osbuild-composer/internal/blueprint"
 	"github.com/osbuild/osbuild-composer/internal/distro"
 	"github.com/osbuild/osbuild-composer/internal/osbuild"
@@ -68,3 +70,23 @@ func TestDistro_Pipeline(t *testing.T) {
 		})
 	}
 }
+
+// Test that all distros are registered properly and that Registry.List() works.
+func TestDistro_RegistryList(t *testing.T) {
+	expected := []string{
+		"fedora-30",
+		"fedora-31",
+		"fedora-32",
+		"rhel-8.1",
+		"rhel-8.2",
+	}
+
+	distros, err := distro.NewDefaultRegistry([]string{"../.."})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if diff := cmp.Diff(distros.List(), expected); diff != "" {
+		t.Errorf("unexpected list of distros: %v", diff)
+	}
+}