Repository: osbuild/osbuild-composer
Pull request #1288 (2021-03-11 12:55:28)

Refactor distro registry to simplify the process of adding a new distro
I made several changes to simplify the process of adding a new distribution, see individual commits for a deeper explanation.

This pull request includes:

- [x] adequate testing for the new functionality or fixed issue (I added some tests)
- [x] adequate documentation informing people about the change (not a functional change)

<!--
Thanks for proposing a change to osbuild-composer!

Please don't remove the above check list. These are things that each pull
request must have before it is merged. It helps maintainers to not forget
anything.

If the reason for ticking any of the boxes is ambiguous, please add a short
note explaining why.

For user-visible changes, "adequate documentation" is an entry describing the
change for users in docs/news. Please refer to docs/news/README.md for details.

In addition, if this pull request fixes a downstream issue, please refer to
test/README.md and add these additional items:

- [ ] 1st commit of any `rhbz#` related PR contains bug reproducer; CI reports FAIL or
- [ ] PR contains automated tests for new functionality and
- [ ] QE has approved reproducer/new tests and
- [ ] Subsequent commits provide bug fixes without modifying the reproducer; CI reports PASS and
- [ ] QE approves this PR; RHBZ status is set to `MODIFIED + Verified=Tested`
-->


ondrejbudai: > Love it! Suggestion for follow-up: assert that all manifest test cases have image-info/manifest/blueprint sections, and that each distro/arch/image-type has a manifest test case. Now it is possible to forget to add/update these, let's make that a tiny bit harder ;)
> Similarly: let's drop support for only generating the manifests without also generating the image tests? We have the new `generate-all-test-cases` script now to help us...

Yeah, I have similar thoughts, thanks!

Patch:
diff --git a/cmd/osbuild-composer/composer.go b/cmd/osbuild-composer/composer.go
index 8e5315cc0..82187afcb 100644
--- a/cmd/osbuild-composer/composer.go
+++ b/cmd/osbuild-composer/composer.go
@@ -14,18 +14,13 @@ import (
 
 	"github.com/osbuild/osbuild-composer/internal/cloudapi"
 	"github.com/osbuild/osbuild-composer/internal/common"
+	"github.com/osbuild/osbuild-composer/internal/distroregistry"
 	"github.com/osbuild/osbuild-composer/internal/jobqueue/fsjobqueue"
 	"github.com/osbuild/osbuild-composer/internal/kojiapi"
 	"github.com/osbuild/osbuild-composer/internal/rpmmd"
 	"github.com/osbuild/osbuild-composer/internal/store"
 	"github.com/osbuild/osbuild-composer/internal/weldr"
 	"github.com/osbuild/osbuild-composer/internal/worker"
-
-	"github.com/osbuild/osbuild-composer/internal/distro"
-	"github.com/osbuild/osbuild-composer/internal/distro/fedora32"
-	"github.com/osbuild/osbuild-composer/internal/distro/fedora33"
-	"github.com/osbuild/osbuild-composer/internal/distro/rhel8"
-	"github.com/osbuild/osbuild-composer/internal/distro/rhel84"
 )
 
 type Composer struct {
@@ -33,7 +28,7 @@ type Composer struct {
 	stateDir string
 	cacheDir string
 	logger   *log.Logger
-	distros  *distro.Registry
+	distros  *distroregistry.Registry
 
 	rpm rpmmd.RPMMD
 
@@ -63,10 +58,7 @@ func NewComposer(config *ComposerConfigFile, stateDir, cacheDir string, logger *
 		return nil, err
 	}
 
-	c.distros, err = distro.NewRegistry(fedora32.New(), fedora33.New(), rhel8.New(), rhel84.New(), rhel84.NewCentos())
-	if err != nil {
-		return nil, fmt.Errorf("Error loading distros: %v", err)
-	}
+	c.distros = distroregistry.NewDefault()
 
 	c.rpm = rpmmd.NewRPMMD(path.Join(c.cacheDir, "rpmmd"), "/usr/libexec/osbuild-composer/dnf-json")
 
diff --git a/cmd/osbuild-pipeline/main.go b/cmd/osbuild-pipeline/main.go
index f380db06a..65ca53adf 100644
--- a/cmd/osbuild-pipeline/main.go
+++ b/cmd/osbuild-pipeline/main.go
@@ -9,10 +9,7 @@ import (
 	"os"
 	"path"
 
-	"github.com/osbuild/osbuild-composer/internal/distro/fedora32"
-	"github.com/osbuild/osbuild-composer/internal/distro/fedora33"
-	"github.com/osbuild/osbuild-composer/internal/distro/rhel8"
-	"github.com/osbuild/osbuild-composer/internal/distro/rhel84"
+	"github.com/osbuild/osbuild-composer/internal/distroregistry"
 
 	"github.com/osbuild/osbuild-composer/internal/blueprint"
 	"github.com/osbuild/osbuild-composer/internal/distro"
@@ -67,11 +64,7 @@ func main() {
 		}
 	}
 
-	distros, err := distro.NewRegistry(fedora32.New(), fedora33.New(), rhel8.New(), rhel84.New(), rhel84.NewCentos())
-	if err != nil {
-		panic(err)
-	}
-
+	distros := distroregistry.NewDefault()
 	d := distros.GetDistro(composeRequest.Distro)
 	if d == nil {
 		_, _ = fmt.Fprintf(os.Stderr, "The provided distribution '%s' is not supported. Use one of these:\n", composeRequest.Distro)
diff --git a/internal/cloudapi/server.go b/internal/cloudapi/server.go
index d931cddd6..a7f692433 100644
--- a/internal/cloudapi/server.go
+++ b/internal/cloudapi/server.go
@@ -15,6 +15,7 @@ import (
 
 	"github.com/osbuild/osbuild-composer/internal/blueprint"
 	"github.com/osbuild/osbuild-composer/internal/distro"
+	"github.com/osbuild/osbuild-composer/internal/distroregistry"
 	"github.com/osbuild/osbuild-composer/internal/rpmmd"
 	"github.com/osbuild/osbuild-composer/internal/target"
 	"github.com/osbuild/osbuild-composer/internal/worker"
@@ -31,11 +32,11 @@ const (
 type Server struct {
 	workers     *worker.Server
 	rpmMetadata rpmmd.RPMMD
-	distros     *distro.Registry
+	distros     *distroregistry.Registry
 }
 
 // NewServer creates a new cloud server
-func NewServer(workers *worker.Server, rpmMetadata rpmmd.RPMMD, distros *distro.Registry) *Server {
+func NewServer(workers *worker.Server, rpmMetadata rpmmd.RPMMD, distros *distroregistry.Registry) *Server {
 	server := &Server{
 		workers:     workers,
 		rpmMetadata: rpmMetadata,
diff --git a/internal/distro/distro.go b/internal/distro/distro.go
index 3db11d5e5..4c6c3f7d7 100644
--- a/internal/distro/distro.go
+++ b/internal/distro/distro.go
@@ -8,7 +8,6 @@ import (
 	"io"
 	"io/ioutil"
 	"os"
-	"sort"
 	"strings"
 
 	"github.com/osbuild/osbuild-composer/internal/blueprint"
@@ -125,57 +124,6 @@ func (m *Manifest) UnmarshalJSON(payload []byte) error {
 	return nil
 }
 
-type Registry struct {
-	distros map[string]Distro
-}
-
-func NewRegistry(distros ...Distro) (*Registry, error) {
-	reg := &Registry{
-		distros: make(map[string]Distro),
-	}
-	for _, distro := range distros {
-		name := distro.Name()
-		if _, exists := reg.distros[name]; exists {
-			return nil, fmt.Errorf("NewRegistry: passed two distros with the same name: %s", distro.Name())
-		}
-		reg.distros[name] = distro
-	}
-	return reg, nil
-}
-
-func (r *Registry) GetDistro(name string) Distro {
-	distro, ok := r.distros[name]
-	if !ok {
-		return nil
-	}
-
-	return distro
-}
-
-// List 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, bool, bool, error) {
-	name, beta, isStream, err := GetHostDistroName()
-	if err != nil {
-		return nil, false, false, err
-	}
-
-	d := r.GetDistro(name)
-	if d == nil {
-		return nil, false, false, errors.New("unknown distro: " + name)
-	}
-
-	return d, beta, isStream, nil
-}
-
 func GetHostDistroName() (string, bool, bool, error) {
 	f, err := os.Open("/etc/os-release")
 	if err != nil {
diff --git a/internal/distroregistry/distroregistry.go b/internal/distroregistry/distroregistry.go
new file mode 100644
index 000000000..e21f14b8c
--- /dev/null
+++ b/internal/distroregistry/distroregistry.go
@@ -0,0 +1,91 @@
+package distroregistry
+
+import (
+	"errors"
+	"fmt"
+	"sort"
+
+	"github.com/osbuild/osbuild-composer/internal/distro"
+	"github.com/osbuild/osbuild-composer/internal/distro/fedora32"
+	"github.com/osbuild/osbuild-composer/internal/distro/fedora33"
+	"github.com/osbuild/osbuild-composer/internal/distro/rhel8"
+	"github.com/osbuild/osbuild-composer/internal/distro/rhel84"
+)
+
+// When adding support for a new distribution, add it here.
+// Note that this is a constant, do not write to this array.
+var supportedDistros = []func() distro.Distro{
+	fedora32.New,
+	fedora33.New,
+	rhel8.New,
+	rhel84.New,
+	rhel84.NewCentos,
+}
+
+type Registry struct {
+	distros map[string]distro.Distro
+}
+
+func New(distros ...distro.Distro) (*Registry, error) {
+	reg := &Registry{
+		distros: make(map[string]distro.Distro),
+	}
+	for _, d := range distros {
+		name := d.Name()
+		if _, exists := reg.distros[name]; exists {
+			return nil, fmt.Errorf("New: passed two distros with the same name: %s", d.Name())
+		}
+		reg.distros[name] = d
+	}
+	return reg, nil
+}
+
+// NewDefault creates a Registry with all distributions supported by
+// osbuild-composer. If you need to add a distribution here, see the
+// supportedDistros variable.
+func NewDefault() *Registry {
+	var distros []distro.Distro
+	for _, distroInitializer := range supportedDistros {
+		distros = append(distros, distroInitializer())
+	}
+
+	registry, err := New(distros...)
+	if err != nil {
+		panic(fmt.Sprintf("two supported distros have the same name, this is a programming error: %v", err))
+	}
+
+	return registry
+}
+
+func (r *Registry) GetDistro(name string) distro.Distro {
+	d, ok := r.distros[name]
+	if !ok {
+		return nil
+	}
+
+	return d
+}
+
+// List returns the names of all distros in a Registry, sorted alphabetically.
+func (r *Registry) List() []string {
+	list := []string{}
+	for _, d := range r.distros {
+		list = append(list, d.Name())
+	}
+	sort.Strings(list)
+	return list
+}
+
+func (r *Registry) FromHost() (distro.Distro, bool, bool, error) {
+	name, beta, isStream, err := distro.GetHostDistroName()
+	if err != nil {
+		return nil, false, false, err
+	}
+
+	d := r.GetDistro(name)
+	if d == nil {
+		return nil, false, false, errors.New("unknown distro: " + name)
+	}
+
+	return d, beta, isStream, nil
+}
diff --git a/internal/kojiapi/server.go b/internal/kojiapi/server.go
index 1c4a5f2b7..78d67a7e0 100644
--- a/internal/kojiapi/server.go
+++ b/internal/kojiapi/server.go
@@ -17,6 +17,7 @@ import (
 
 	"github.com/osbuild/osbuild-composer/internal/blueprint"
 	"github.com/osbuild/osbuild-composer/internal/distro"
+	"github.com/osbuild/osbuild-composer/internal/distroregistry"
 	"github.com/osbuild/osbuild-composer/internal/kojiapi/api"
 	"github.com/osbuild/osbuild-composer/internal/rpmmd"
 	"github.com/osbuild/osbuild-composer/internal/worker"
@@ -27,11 +28,11 @@ type Server struct {
 	logger      *log.Logger
 	workers     *worker.Server
 	rpmMetadata rpmmd.RPMMD
-	distros     *distro.Registry
+	distros     *distroregistry.Registry
 }
 
 // NewServer creates a new koji server
-func NewServer(logger *log.Logger, workers *worker.Server, rpmMetadata rpmmd.RPMMD, distros *distro.Registry) *Server {
+func NewServer(logger *log.Logger, workers *worker.Server, rpmMetadata rpmmd.RPMMD, distros *distroregistry.Registry) *Server {
 	s := &Server{
 		logger:      logger,
 		workers:     workers,
diff --git a/internal/mocks/distro/distro_mock.go b/internal/mocks/distro/distro_mock.go
index ce65d5005..59dcee0fc 100644
--- a/internal/mocks/distro/distro_mock.go
+++ b/internal/mocks/distro/distro_mock.go
@@ -1,14 +1,14 @@
 package distro_mock
 
 import (
-	"github.com/osbuild/osbuild-composer/internal/distro"
 	"github.com/osbuild/osbuild-composer/internal/distro/fedoratest"
+	"github.com/osbuild/osbuild-composer/internal/distroregistry"
 )
 
-func NewDefaultRegistry() (*distro.Registry, error) {
+func NewDefaultRegistry() (*distroregistry.Registry, error) {
 	ftest := fedoratest.New()
 	if ftest == nil {
 		panic("Attempt to register Fedora test failed")
 	}
-	return distro.NewRegistry(ftest)
+	return distroregistry.New(ftest)
 }


Test patch:
diff --git a/internal/distro/distro_test.go b/internal/distro/distro_test.go
index cc1250b16..c94003912 100644
--- a/internal/distro/distro_test.go
+++ b/internal/distro/distro_test.go
@@ -3,37 +3,16 @@ package distro_test
 import (
 	"testing"
 
-	"github.com/stretchr/testify/require"
-
-	"github.com/osbuild/osbuild-composer/internal/distro"
 	"github.com/osbuild/osbuild-composer/internal/distro/distro_test_common"
-	"github.com/osbuild/osbuild-composer/internal/distro/fedora32"
-	"github.com/osbuild/osbuild-composer/internal/distro/fedora33"
-	"github.com/osbuild/osbuild-composer/internal/distro/rhel8"
-	"github.com/osbuild/osbuild-composer/internal/distro/rhel84"
+	"github.com/osbuild/osbuild-composer/internal/distroregistry"
 )
 
 func TestDistro_Manifest(t *testing.T) {
+
 	distro_test_common.TestDistro_Manifest(
 		t,
 		"../../test/data/manifests/",
 		"*",
-		fedora32.New(), fedora33.New(), rhel8.New(), rhel84.New(), rhel84.NewCentos(),
+		distroregistry.NewDefault(),
 	)
 }
-
-// Test that all distros are registered properly and that Registry.List() works.
-func TestDistro_RegistryList(t *testing.T) {
-	expected := []string{
-		"centos-8",
-		"fedora-32",
-		"fedora-33",
-		"rhel-8",
-		"rhel-84",
-	}
-
-	distros, err := distro.NewRegistry(fedora32.New(), fedora33.New(), rhel8.New(), rhel84.New(), rhel84.NewCentos())
-	require.NoError(t, err)
-
-	require.Equalf(t, expected, distros.List(), "unexpected list of distros")
-}
diff --git a/internal/distro/distro_test_common/distro_test_common.go b/internal/distro/distro_test_common/distro_test_common.go
index 4121cc27b..86a2e3727 100644
--- a/internal/distro/distro_test_common/distro_test_common.go
+++ b/internal/distro/distro_test_common/distro_test_common.go
@@ -9,16 +9,18 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
 	"github.com/osbuild/osbuild-composer/internal/blueprint"
 	"github.com/osbuild/osbuild-composer/internal/distro"
+	"github.com/osbuild/osbuild-composer/internal/distroregistry"
 	"github.com/osbuild/osbuild-composer/internal/rpmmd"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )
 
 const RandomTestSeed = 0
 
-func TestDistro_Manifest(t *testing.T, pipelinePath string, prefix string, distros ...distro.Distro) {
+func TestDistro_Manifest(t *testing.T, pipelinePath string, prefix string, registry *distroregistry.Registry) {
 	assert := assert.New(t)
 	fileNames, err := filepath.Glob(filepath.Join(pipelinePath, prefix))
 	assert.NoErrorf(err, "Could not read pipelines directory '%s': %v", pipelinePath, err)
@@ -64,9 +66,8 @@ func TestDistro_Manifest(t *testing.T, pipelinePath string, prefix string, distr
 			}
 		}
 		t.Run(path.Base(fileName), func(t *testing.T) {
-			distros, err := distro.NewRegistry(distros...)
 			require.NoError(t, err)
-			d := distros.GetDistro(tt.ComposeRequest.Distro)
+			d := registry.GetDistro(tt.ComposeRequest.Distro)
 			if d == nil {
 				t.Errorf("unknown distro: %v", tt.ComposeRequest.Distro)
 				return
diff --git a/internal/distro/fedora32/distro_test.go b/internal/distro/fedora32/distro_test.go
index 36e4aef0c..1e115ebc3 100644
--- a/internal/distro/fedora32/distro_test.go
+++ b/internal/distro/fedora32/distro_test.go
@@ -3,11 +3,12 @@ package fedora32_test
 import (
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+
 	"github.com/osbuild/osbuild-composer/internal/blueprint"
 	"github.com/osbuild/osbuild-composer/internal/distro"
 	"github.com/osbuild/osbuild-composer/internal/distro/distro_test_common"
 	"github.com/osbuild/osbuild-composer/internal/distro/fedora32"
-	"github.com/stretchr/testify/assert"
 )
 
 func TestFilenameFromType(t *testing.T) {
@@ -295,10 +296,6 @@ func TestImageType_BasePackages(t *testing.T) {
 	}
 }
 
-func TestDistro_Manifest(t *testing.T) {
-	distro_test_common.TestDistro_Manifest(t, "../../../test/data/manifests/", "fedora_32*", fedora32.New())
-}
-
 // Check that Manifest() function returns an error for unsupported
 // configurations.
 func TestDistro_ManifestError(t *testing.T) {
diff --git a/internal/distro/fedora33/distro_test.go b/internal/distro/fedora33/distro_test.go
index e9ae5e2ce..e2133f815 100644
--- a/internal/distro/fedora33/distro_test.go
+++ b/internal/distro/fedora33/distro_test.go
@@ -3,11 +3,12 @@ package fedora33_test
 import (
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+
 	"github.com/osbuild/osbuild-composer/internal/blueprint"
 	"github.com/osbuild/osbuild-composer/internal/distro"
 	"github.com/osbuild/osbuild-composer/internal/distro/distro_test_common"
 	"github.com/osbuild/osbuild-composer/internal/distro/fedora33"
-	"github.com/stretchr/testify/assert"
 )
 
 func TestFilenameFromType(t *testing.T) {
@@ -301,10 +302,6 @@ func TestImageType_BasePackages(t *testing.T) {
 	}
 }
 
-func TestDistro_Manifest(t *testing.T) {
-	distro_test_common.TestDistro_Manifest(t, "../../../test/data/manifests/", "fedora_33*", fedora33.New())
-}
-
 // Check that Manifest() function returns an error for unsupported
 // configurations.
 func TestDistro_ManifestError(t *testing.T) {
diff --git a/internal/distro/rhel8/distro_test.go b/internal/distro/rhel8/distro_test.go
index 6305d07ac..75a73e1f8 100644
--- a/internal/distro/rhel8/distro_test.go
+++ b/internal/distro/rhel8/distro_test.go
@@ -3,11 +3,12 @@ package rhel8_test
 import (
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+
 	"github.com/osbuild/osbuild-composer/internal/blueprint"
 	"github.com/osbuild/osbuild-composer/internal/distro"
 	"github.com/osbuild/osbuild-composer/internal/distro/distro_test_common"
 	"github.com/osbuild/osbuild-composer/internal/distro/rhel8"
-	"github.com/stretchr/testify/assert"
 )
 
 func TestFilenameFromType(t *testing.T) {
@@ -354,10 +355,6 @@ func TestImageType_BasePackages(t *testing.T) {
 	}
 }
 
-func TestDistro_Manifest(t *testing.T) {
-	distro_test_common.TestDistro_Manifest(t, "../../../test/data/manifests/", "rhel_8-*", rhel8.New())
-}
-
 // Check that Manifest() function returns an error for unsupported
 // configurations.
 func TestDistro_ManifestError(t *testing.T) {
diff --git a/internal/distro/rhel84/distro_test.go b/internal/distro/rhel84/distro_test.go
index 741dff5a2..8827b337b 100644
--- a/internal/distro/rhel84/distro_test.go
+++ b/internal/distro/rhel84/distro_test.go
@@ -414,11 +414,6 @@ func TestImageType_BasePackages(t *testing.T) {
 	}
 }
 
-func TestDistro_Manifest(t *testing.T) {
-	distro_test_common.TestDistro_Manifest(t, "../../../test/data/manifests/", "rhel_84*", rhel84.New())
-	distro_test_common.TestDistro_Manifest(t, "../../../test/data/manifests/", "centos_8*", rhel84.NewCentos())
-}
-
 // Check that Manifest() function returns an error for unsupported
 // configurations.
 func TestDistro_ManifestError(t *testing.T) {
diff --git a/internal/distroregistry/distroregistry_test.go b/internal/distroregistry/distroregistry_test.go
new file mode 100644
index 000000000..ebe6a8665
--- /dev/null
+++ b/internal/distroregistry/distroregistry_test.go
@@ -0,0 +1,36 @@
+package distroregistry
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/osbuild/osbuild-composer/internal/distro/rhel8"
+)
+
+// Test that all distros are registered properly and that Registry.List() works.
+func TestRegistry_List(t *testing.T) {
+	// build expected distros
+	var expected []string
+	for _, distroInitializer := range supportedDistros {
+		d := distroInitializer()
+		expected = append(expected, d.Name())
+	}
+
+	distros := NewDefault()
+
+	require.ElementsMatch(t, expected, distros.List(), "unexpected list of distros")
+}
+
+func TestRegistry_GetDistro(t *testing.T) {
+	distros := NewDefault()
+
+	t.Run("distro exists", func(t *testing.T) {
+		expectedDistro := rhel8.New()
+		require.Equal(t, expectedDistro.Name(), distros.GetDistro(expectedDistro.Name()).Name())
+	})
+
+	t.Run("distro doesn't exist", func(t *testing.T) {
+		require.Nil(t, distros.GetDistro("toucan-os"))
+	})
+}