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

data: copy all repositories from osbuild-composer to images #1112

Merged
merged 2 commits into from
Jan 8, 2025

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented Dec 19, 2024

This PR is an attempt to have my cake and eat it. In a nutshell:

  1. Nothing^Wvery little changes for composer when handling
    the repos. They are still normal files but instead of getting
    them from the repositories/ subdir the files are now
    part of vendor/github.com/osbuild/images/data/repositories
    (i.e. we just modify the specfile to copy from there, otherwise
    all the same logic)
  2. image-builder-cli can ship the defaults directly in the binary
    which means it is possible to just "go run github.com/..image-builder-cli"
  3. If nothing (go) imports the repos no extra data is added to the
    binaries
  4. We still support overrides by first using the search path for
    real files and then falling back to the buildin repos (not done yet) [1]

Except for (slightly) higher complexity I see no real downsides
currently, but maybe I'm in love too much with my idea, have
a (critical!) look please.

[1] Happy to fix reporegistry but I don't want to spend too much time
upfront if the idea is not liked


This commit moves the repositories from osbuild-composer to
images. The rational is that we need a central place to host
the repositories so that they can be shared between osbuild-composer
and image-builder-cli.

The other reason is that conceptually images is (arguably) be a
better place because it knows how to build osbuild manifests and
it can only build those for distro releases it knows about. The
filenames are already part of the API of images (as it derives
the distro name from the filename) and it is impossible to add
new distros (except maybe point releases) without also updateing
images to deal with the details of the new distro.


data: make repositories available via go:embed

This commit allows us to discover/load repositories via go:embed.
Note that unlike the testrepos.go code this one does not provide
a New() helper. Instead we will modify the RepoRegistry to
look for the search paths for repo configuration first and then
fallback to the buildin repos (unless disabled via a flag).

Embedding also allows us an easy transition path in composer. The
rpm specfile will simply need to copy the repository data from
vendor/github.com/osbuild/images/data/repositories instead of
the current location and we can delete the repositories already
covered here (composer will still carry the -no-aux-keys repos
and the symlinks (which sadly cannot be expressed via go:embed)).

This commit moves the repositories from `osbuild-composer` to
images. The rational is that we need a central place to host
the repositories so that they can be shared between osbuild-composer
and image-builder-cli.

The other reason is that conceptually images is (arguably) be a
better place because it knows how to build osbuild manifests and
it can only build those for distro releases it knows about. The
filenames are already part of the API of images (as it derives
the distro name from the filename) and it is impossible to add
new distros (except maybe point releases) without also updateing
images to deal with the details of the new distro.
supakeen
supakeen previously approved these changes Jan 7, 2025
Copy link
Member

@supakeen supakeen left a comment

Choose a reason for hiding this comment

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

Needs linter appeasement, otherwise this approach is fine to me as long as we overlay the 'current data paths' from osbuild-composer over the embedFS in the other systems.

@mvo5 mvo5 requested a review from ondrejbudai January 7, 2025 13:52
achilleas-k
achilleas-k previously approved these changes Jan 8, 2025
thozza
thozza previously approved these changes Jan 8, 2025
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.

LGTM.

I think embedding -no-aux-keys may be helpful if we end up wanting to use embedded repos on newer distros to build an image of an old one.

@mvo5
Copy link
Contributor Author

mvo5 commented Jan 8, 2025

LGTM.

I think embedding -no-aux-keys may be helpful if we end up wanting to use embedded repos on newer distros to build an image of an old one.

Thanks! If I understand this correctly we probably want to embed the -no-aux-keys then here by default as this seems to be the common use-case (that we run image-builder-cli or similar on a new distro/container and want to build an older one). If I got this right I will switch it around (need to fix the linter anyway) but happy to leave as is if it needs more discussion, we can always tweak it in a followup :)

@thozza
Copy link
Member

thozza commented Jan 8, 2025

LGTM.
I think embedding -no-aux-keys may be helpful if we end up wanting to use embedded repos on newer distros to build an image of an old one.

Thanks! If I understand this correctly we probably want to embed the -no-aux-keys then here by default as this seems to be the common use-case (that we run image-builder-cli or similar on a new distro/container and want to build an older one). If I got this right I will switch it around (need to fix the linter anyway) but happy to leave as is if it needs more discussion, we can always tweak it in a followup :)

A followup is IMO better.

And yes, it's a common problem with RHEL releases, where the newer ones have stricter requirements on the crypto algorithms, while old releases use keys that end up being invalid on newer releases.

ondrejbudai
ondrejbudai previously approved these changes Jan 8, 2025
Copy link
Member

@ondrejbudai ondrejbudai left a comment

Choose a reason for hiding this comment

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

Thanks Michael for opening this, and others for reviewing! ❤️

@mvo5 mvo5 changed the title [RFC] data: copy all repositories from osbuild-composer to images data: copy all repositories from osbuild-composer to images Jan 8, 2025
This commit allows us to discover/load repositories via `go:embed`.
Note that unlike the `testrepos.go` code this one does not provide
a `New()` helper. Instead we will modify the `RepoRegistry` to
look for the search paths for repo configuration first and then
fallback to the buildin repos (unless disabled via a flag).

Embedding also allows us an easy transition path in composer. The
rpm specfile will simply need to copy the repository data from
`vendor/github.com/osbuild/images/data/repositories` instead of
the current location and we can delete the repositories already
covered here (composer will still carry the `-no-aux-keys` repos
and the symlinks (which sadly cannot be expressed via go:embed)).
@mvo5 mvo5 dismissed stale reviews from ondrejbudai, thozza, achilleas-k, and supakeen via 4f0210c January 8, 2025 12:30
@mvo5 mvo5 force-pushed the add-repos-from-composer branch from 37e61f5 to 4f0210c Compare January 8, 2025 12:30
@mvo5
Copy link
Contributor Author

mvo5 commented Jan 8, 2025

I (hopefully) fixed the silly linter mistake I made, once this is in I willdo followups with:

  1. integrate with reporegisty (optionally, i.e. including an option to not use embedded repos at all) and with the correct overrides (i.e. /etc wins over /usr wins over build-in)
  2. integrate into image-builder-cli (hopefully as rebuild is enough if we get reporegistry right)
  3. update osbuild-composer to copy the shared repos via vendor/... instead of having copies in the spec file

Copy link
Member

@ondrejbudai ondrejbudai left a comment

Choose a reason for hiding this comment

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

Let's eat the cake.

@mvo5 mvo5 enabled auto-merge January 8, 2025 13:47
@mvo5 mvo5 added this pull request to the merge queue Jan 8, 2025
Merged via the queue into osbuild:main with commit 5aaf806 Jan 8, 2025
17 of 18 checks passed
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.

5 participants