From 3c38093245c5f404e6476bcc53d5c66a7696ed67 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Wed, 29 Jan 2025 10:13:43 +0100 Subject: [PATCH 1/2] reporegistry: expand `New()`, unexport `loadAllRepositories` This commit changes `reporegistry.New()` to take a new parameter `repoConfigFS []fs.FS`. With that we can unexport the public `LoadAllRepositories{,FromFS}` as the only user for those is `composer` and that can simply use `.New()`. Note that we could also change `New()` to just take a single `repoConfig []fs.FS` but the downside of this is that the error reporting becomes more awkward, i.e. to report what paths where configured would (probably) require to build them manually via `fs.FS.Open(".").Stat().Name()` (but I have not tested this). --- cmd/build/main.go | 7 +------ pkg/reporegistry/error.go | 8 ++++++-- pkg/reporegistry/reporegistry.go | 10 ++++++---- pkg/reporegistry/repository.go | 19 ++++++++++--------- pkg/reporegistry/repository_test.go | 4 ++-- test/data/repositories/testrepos.go | 6 +----- 6 files changed, 26 insertions(+), 28 deletions(-) diff --git a/cmd/build/main.go b/cmd/build/main.go index 4fca4de686..ee82a6251e 100644 --- a/cmd/build/main.go +++ b/cmd/build/main.go @@ -93,12 +93,7 @@ func run() error { } overrideRepos = repoConfig[archName] } else { - // HACK: reporegistry hardcodes adding the "repositories" - // dir to the "repoConfigPaths" but the interface of - // "images" does not expect this. - // XXX: should we fix this in reporegistry? - repositories = filepath.Join(repositories, "..") - reporeg, err = reporegistry.New([]string{repositories}) + reporeg, err = reporegistry.New([]string{repositories}, nil) if err != nil { return fmt.Errorf("failed to load repositories from %q: %w", repositories, err) } diff --git a/pkg/reporegistry/error.go b/pkg/reporegistry/error.go index 7e0938d7fe..34e7bb080b 100644 --- a/pkg/reporegistry/error.go +++ b/pkg/reporegistry/error.go @@ -1,13 +1,17 @@ package reporegistry -import "fmt" +import ( + "fmt" + "io/fs" +) // NoReposLoadedError is an error type that is returned when no repositories // are loaded from the given paths. type NoReposLoadedError struct { Paths []string + FSes []fs.FS } func (e *NoReposLoadedError) Error() string { - return fmt.Sprintf("no repositories found in the given paths: %v", e.Paths) + return fmt.Sprintf("no repositories found in the given paths: %v/%v", e.Paths, e.FSes) } diff --git a/pkg/reporegistry/reporegistry.go b/pkg/reporegistry/reporegistry.go index 7a15e042ad..2cd02c26d3 100644 --- a/pkg/reporegistry/reporegistry.go +++ b/pkg/reporegistry/reporegistry.go @@ -2,6 +2,7 @@ package reporegistry import ( "fmt" + "io/fs" "github.com/osbuild/images/pkg/distroidparser" "github.com/osbuild/images/pkg/rpmmd" @@ -14,10 +15,11 @@ type RepoRegistry struct { repos rpmmd.DistrosRepoConfigs } -// New returns a new RepoRegistry instance with the data -// loaded from the given repoConfigPaths -func New(repoConfigPaths []string) (*RepoRegistry, error) { - repositories, err := LoadAllRepositories(repoConfigPaths) +// New returns a new RepoRegistry instance with the data loaded from +// the given repoConfigPaths and repoConfigFS instance. The order is +// important here, first the paths are tried, then the FSes +func New(repoConfigPaths []string, repoConfigFS []fs.FS) (*RepoRegistry, error) { + repositories, err := loadAllRepositories(repoConfigPaths, repoConfigFS) if err != nil { return nil, err } diff --git a/pkg/reporegistry/repository.go b/pkg/reporegistry/repository.go index 4706724bef..0cd6d82293 100644 --- a/pkg/reporegistry/repository.go +++ b/pkg/reporegistry/repository.go @@ -12,23 +12,24 @@ import ( "github.com/osbuild/images/pkg/rpmmd" ) -// LoadAllRepositories loads all repositories for given distros from the given list of paths. +// loadAllRepositories loads all repositories for given distros from the given list of paths. // Behavior is the same as with the LoadRepositories() method. -func LoadAllRepositories(confPaths []string) (rpmmd.DistrosRepoConfigs, error) { - var confFSes []fs.FS +func loadAllRepositories(confPaths []string, confFSes []fs.FS) (rpmmd.DistrosRepoConfigs, error) { + var mergedFSes []fs.FS for _, confPath := range confPaths { - confFSes = append(confFSes, os.DirFS(filepath.Join(confPath, "repositories"))) + mergedFSes = append(mergedFSes, os.DirFS(filepath.Join(confPath, "repositories"))) } + mergedFSes = append(mergedFSes, confFSes...) - distrosRepoConfigs, err := LoadAllRepositoriesFromFS(confFSes) + distrosRepoConfigs, err := loadAllRepositoriesFromFS(mergedFSes) if len(distrosRepoConfigs) == 0 { - return nil, &NoReposLoadedError{confPaths} + return nil, &NoReposLoadedError{confPaths, confFSes} } return distrosRepoConfigs, err } -func LoadAllRepositoriesFromFS(confPaths []fs.FS) (rpmmd.DistrosRepoConfigs, error) { +func loadAllRepositoriesFromFS(confPaths []fs.FS) (rpmmd.DistrosRepoConfigs, error) { distrosRepoConfigs := rpmmd.DistrosRepoConfigs{} for _, confPath := range confPaths { @@ -95,7 +96,7 @@ func LoadRepositories(confPaths []string, distro string) (map[string][]rpmmd.Rep for _, confPath := range confPaths { var err error - repoConfigs, err = rpmmd.LoadRepositoriesFromFile(confPath + path) + repoConfigs, err = rpmmd.LoadRepositoriesFromFile(filepath.Join(confPath, path)) if os.IsNotExist(err) { continue } else if err != nil { @@ -109,7 +110,7 @@ func LoadRepositories(confPaths []string, distro string) (map[string][]rpmmd.Rep } if repoConfigs == nil { - return nil, &NoReposLoadedError{confPaths} + return nil, &NoReposLoadedError{confPaths, nil} } return repoConfigs, nil diff --git a/pkg/reporegistry/repository_test.go b/pkg/reporegistry/repository_test.go index 0bcf75f63f..f905dd8258 100644 --- a/pkg/reporegistry/repository_test.go +++ b/pkg/reporegistry/repository_test.go @@ -280,7 +280,7 @@ func Test_LoadAllRepositories(t *testing.T) { confPaths := getConfPaths(t) - distroReposMap, err := LoadAllRepositories(confPaths) + distroReposMap, err := loadAllRepositories(confPaths, nil) assert.NotNil(t, distroReposMap) assert.Nil(t, err) assert.Equal(t, len(distroReposMap), len(expectedReposMap)) @@ -307,7 +307,7 @@ func TestLoadRepositoriesLogging(t *testing.T) { logrus.AddHook(logHook) confPaths := getConfPaths(t) - _, err := LoadAllRepositories(confPaths) + _, err := loadAllRepositories(confPaths, nil) require.NoError(t, err) needle := "Loaded repository configuration file: rhel-8.10.json" assert.True(t, slices.ContainsFunc(logHook.AllEntries(), func(entry *logrus.Entry) bool { diff --git a/test/data/repositories/testrepos.go b/test/data/repositories/testrepos.go index f9c72a4a47..a780706af3 100644 --- a/test/data/repositories/testrepos.go +++ b/test/data/repositories/testrepos.go @@ -11,9 +11,5 @@ import ( var FS embed.FS func New() (*reporegistry.RepoRegistry, error) { - repositories, err := reporegistry.LoadAllRepositoriesFromFS([]fs.FS{FS}) - if err != nil { - return nil, err - } - return reporegistry.NewFromDistrosRepoConfigs(repositories), nil + return reporegistry.New(nil, []fs.FS{FS}) } From 6191f472cb2f46e4427735d297a005de1704f456 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Wed, 29 Jan 2025 10:30:07 +0100 Subject: [PATCH 2/2] reporegistry: do not prepend `repositories` in confPaths This commit breaks the API by changing the confPaths loading in reporegistry to stop prepending `repositories/`. This makes it easier to use this interface in e.g. `cmd/build`. Note that this requires changes in the upstream projecs, notably osbuild-composer and image-builder-cli. --- pkg/reporegistry/reporegistry.go | 5 ++++- pkg/reporegistry/repository.go | 7 +++++-- pkg/reporegistry/repository_test.go | 4 ++-- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/pkg/reporegistry/reporegistry.go b/pkg/reporegistry/reporegistry.go index 2cd02c26d3..7e796fd028 100644 --- a/pkg/reporegistry/reporegistry.go +++ b/pkg/reporegistry/reporegistry.go @@ -17,7 +17,10 @@ type RepoRegistry struct { // New returns a new RepoRegistry instance with the data loaded from // the given repoConfigPaths and repoConfigFS instance. The order is -// important here, first the paths are tried, then the FSes +// important here, first the paths are tried, then the FSes. +// +// Note that the confPaths must point directly to the directory with +// the json repo files. func New(repoConfigPaths []string, repoConfigFS []fs.FS) (*RepoRegistry, error) { repositories, err := loadAllRepositories(repoConfigPaths, repoConfigFS) if err != nil { diff --git a/pkg/reporegistry/repository.go b/pkg/reporegistry/repository.go index 0cd6d82293..2104b6fb38 100644 --- a/pkg/reporegistry/repository.go +++ b/pkg/reporegistry/repository.go @@ -18,7 +18,7 @@ func loadAllRepositories(confPaths []string, confFSes []fs.FS) (rpmmd.DistrosRep var mergedFSes []fs.FS for _, confPath := range confPaths { - mergedFSes = append(mergedFSes, os.DirFS(filepath.Join(confPath, "repositories"))) + mergedFSes = append(mergedFSes, os.DirFS(confPath)) } mergedFSes = append(mergedFSes, confFSes...) @@ -90,9 +90,12 @@ func loadAllRepositoriesFromFS(confPaths []fs.FS) (rpmmd.DistrosRepoConfigs, err // If there are duplicate distro repositories definitions found in multiple paths, the first // encounter is preferred. For this reason, the order of paths in the passed list should // reflect the desired preference. +// +// Note that the confPaths must point directly to the directory with +// the json repo files. func LoadRepositories(confPaths []string, distro string) (map[string][]rpmmd.RepoConfig, error) { var repoConfigs map[string][]rpmmd.RepoConfig - path := "/repositories/" + distro + ".json" + path := distro + ".json" for _, confPath := range confPaths { var err error diff --git a/pkg/reporegistry/repository_test.go b/pkg/reporegistry/repository_test.go index f905dd8258..30a29b3951 100644 --- a/pkg/reporegistry/repository_test.go +++ b/pkg/reporegistry/repository_test.go @@ -19,8 +19,8 @@ import ( func getConfPaths(t *testing.T) []string { confPaths := []string{ - "./test/confpaths/priority1", - "./test/confpaths/priority2", + "./test/confpaths/priority1/repositories", + "./test/confpaths/priority2/repositories", } var absConfPaths []string