Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reporegistry: do not prepend repositories in confPaths #1179

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented Jan 29, 2025

[draft as this has too few tests, but I want to get feedback
before writing them]

This PR changes the API for reporegistry in two incompatible
ways. The important change is that we want to drop the
prepending of repositories when loading from confPaths.

Doing this on its own has the disadvantage of breaking
the API without breaking the builds. So this includes a
small tweak to allow adding repos via fs.FS directly
which will mean the code in ibcli will become simpler:

diff --git a/cmd/image-builder/repos.go b/cmd/image-builder/repos.go
index bc238dc..5d16a95 100644
--- a/cmd/image-builder/repos.go
+++ b/cmd/image-builder/repos.go
@@ -2,8 +2,6 @@ package main
 
 import (
        "io/fs"
-       "os"
-       "path/filepath"
 
        "github.com/osbuild/images/data/repositories"
        "github.com/osbuild/images/pkg/reporegistry"
@@ -27,17 +25,6 @@ var newRepoRegistry = func(dataDir string) (*reporegistry.RepoRegistry, error) {
                dataDirs = defaultDataDirs
        }
 
-       // XXX: think about sharing this with reporegistry?
-       var fses []fs.FS
-       for _, d := range dataDirs {
-               fses = append(fses, os.DirFS(filepath.Join(d, "repositories")))
-       }
-       fses = append(fses, repos.FS)
-
        // XXX: should we support disabling the build-ins somehow?
-       conf, err := reporegistry.LoadAllRepositoriesFromFS(fses)
-       if err != nil {
-               return nil, err
-       }
-       return reporegistry.NewFromDistrosRepoConfigs(conf), nil
+       return reporegistry.New(dataDirs, []fs.FS{repos.FS})
 }

And for composer it will just be adding a nil to the New()
call.

Note that this also allows us to unexport the LoadAllRepositories
repository loading as this is only used by composer or ibcli.


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.


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

mvo5 added 2 commits January 29, 2025 10:28
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).
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.
@mvo5 mvo5 requested a review from thozza January 29, 2025 09:55
mvo5 added a commit to mvo5/osbuild-composer that referenced this pull request Jan 29, 2025
This commit changes cmd/composer to use the `reporegistry.New`
call again. This is a preparation for
osbuild/images#1179

And undoes parts of osbuild#4378
but that is no longer necessary because in
osbuild/images#946 the error is now passed
on from `reporegistry.New()` in the same way as from `LoadRepositories()`.
Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea of the change looks good to me, but the code needs some fixes for tests to pass...

thozza pushed a commit to osbuild/osbuild-composer that referenced this pull request Jan 29, 2025
This commit changes cmd/composer to use the `reporegistry.New`
call again. This is a preparation for
osbuild/images#1179

And undoes parts of #4378
but that is no longer necessary because in
osbuild/images#946 the error is now passed
on from `reporegistry.New()` in the same way as from `LoadRepositories()`.
mvo5 added a commit to mvo5/osbuild-composer that referenced this pull request Jan 29, 2025
This commit updates the cloudapi test that uses the real repositories
to use the version of those from the "images" library. Composer
no longer carries the default repos.

Note that this can most likely be simplified once
osbuild/images#1179
is merged.
mvo5 added a commit to mvo5/osbuild-composer that referenced this pull request Jan 29, 2025
This commit updates the cloudapi test that uses the real repositories
to use the version of those from the "images" library. Composer
no longer carries the default repos.

Note that this can most likely be simplified once
osbuild/images#1179
is merged.
mvo5 added a commit to mvo5/osbuild-composer that referenced this pull request Jan 29, 2025
This commit updates the cloudapi test that uses the real repositories
to use the version of those from the "images" library. Composer
no longer carries the default repos.

Note that this can most likely be simplified once
osbuild/images#1179
is merged.
mvo5 added a commit to mvo5/osbuild-composer that referenced this pull request Jan 30, 2025
This commit updates the cloudapi test that uses the real repositories
to use the version of those from the "images" library. Composer
no longer carries the default repos.

Note that this can most likely be simplified once
osbuild/images#1179
is merged.
mvo5 added a commit to mvo5/osbuild-composer that referenced this pull request Jan 30, 2025
This commit updates the cloudapi test that uses the real repositories
to use the version of those from the "images" library. Composer
no longer carries the default repos.

Note that this can most likely be simplified once
osbuild/images#1179
is merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants