From dd4db353e269cd3fd6589922f06cffe56a45d7ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Budai?= Date: Thu, 11 Mar 2021 12:30:58 +0100 Subject: [PATCH] distro: move Registry to its own distroregistry package MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit My goal is to add a method to distroregistry to return Registry with all supported distributions. This way, all supported distributions would be defined only on one place. To achieve this, the Registry must live outside the distro package because the distro implementation depends on it and this would create a circular dependency unsupported by Go. Signed-off-by: Ondřej Budai --- cmd/osbuild-composer/composer.go | 6 +- cmd/osbuild-pipeline/main.go | 3 +- internal/cloudapi/server.go | 5 +- internal/distro/distro.go | 52 ---------------- internal/distro/distro_test.go | 19 ------ .../distro_test_common/distro_test_common.go | 10 ++-- internal/distroregistry/distroregistry.go | 60 +++++++++++++++++++ .../distroregistry/distroregistry_test.go | 29 +++++++++ internal/kojiapi/server.go | 5 +- internal/mocks/distro/distro_mock.go | 6 +- 10 files changed, 109 insertions(+), 86 deletions(-) create mode 100644 internal/distroregistry/distroregistry.go create mode 100644 internal/distroregistry/distroregistry_test.go diff --git a/cmd/osbuild-composer/composer.go b/cmd/osbuild-composer/composer.go index 8e5315cc0..721c73127 100644 --- a/cmd/osbuild-composer/composer.go +++ b/cmd/osbuild-composer/composer.go @@ -14,6 +14,7 @@ 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" @@ -21,7 +22,6 @@ import ( "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" @@ -33,7 +33,7 @@ type Composer struct { stateDir string cacheDir string logger *log.Logger - distros *distro.Registry + distros *distroregistry.Registry rpm rpmmd.RPMMD @@ -63,7 +63,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()) + c.distros, err = distroregistry.New(fedora32.New(), fedora33.New(), rhel8.New(), rhel84.New(), rhel84.NewCentos()) if err != nil { return nil, fmt.Errorf("Error loading distros: %v", err) } diff --git a/cmd/osbuild-pipeline/main.go b/cmd/osbuild-pipeline/main.go index f380db06a..a3fa4dfd3 100644 --- a/cmd/osbuild-pipeline/main.go +++ b/cmd/osbuild-pipeline/main.go @@ -13,6 +13,7 @@ import ( "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,7 +68,7 @@ func main() { } } - distros, err := distro.NewRegistry(fedora32.New(), fedora33.New(), rhel8.New(), rhel84.New(), rhel84.NewCentos()) + distros, err := distroregistry.New(fedora32.New(), fedora33.New(), rhel8.New(), rhel84.New(), rhel84.NewCentos()) if err != nil { panic(err) } 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/distro/distro_test.go b/internal/distro/distro_test.go index cc1250b16..9e4590c8f 100644 --- a/internal/distro/distro_test.go +++ b/internal/distro/distro_test.go @@ -3,9 +3,6 @@ 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" @@ -21,19 +18,3 @@ func TestDistro_Manifest(t *testing.T) { fedora32.New(), fedora33.New(), rhel8.New(), rhel84.New(), rhel84.NewCentos(), ) } - -// 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..a32bb38d0 100644 --- a/internal/distro/distro_test_common/distro_test_common.go +++ b/internal/distro/distro_test_common/distro_test_common.go @@ -9,11 +9,13 @@ import ( "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/rpmmd" "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" ) const RandomTestSeed = 0 @@ -64,7 +66,7 @@ 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...) + distros, err := distroregistry.New(distros...) require.NoError(t, err) d := distros.GetDistro(tt.ComposeRequest.Distro) if d == nil { diff --git a/internal/distroregistry/distroregistry.go b/internal/distroregistry/distroregistry.go new file mode 100644 index 000000000..c99e75f47 --- /dev/null +++ b/internal/distroregistry/distroregistry.go @@ -0,0 +1,60 @@ +package distroregistry + +import ( + "errors" + "fmt" + "sort" + + "github.com/osbuild/osbuild-composer/internal/distro" +) + +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 +} + +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/distroregistry/distroregistry_test.go b/internal/distroregistry/distroregistry_test.go new file mode 100644 index 000000000..a90a92a45 --- /dev/null +++ b/internal/distroregistry/distroregistry_test.go @@ -0,0 +1,29 @@ +package distroregistry_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "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" +) + +// Test that all distros are registered properly and that Registry.List() works. +func TestRegistry_List(t *testing.T) { + expected := []string{ + "centos-8", + "fedora-32", + "fedora-33", + "rhel-8", + "rhel-84", + } + + distros, err := distroregistry.New(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/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) }