reporegistry: various cleanups #1980

mvo5 · 2025-10-30T10:51:37Z

This started as a quick drive-by to tweak things I noticed while working
on the tests for osbuild/image-builder-cli#360

It grew into more when I dove into the API of reporegitry and how we use.

I would like to do some followups (that will require change in composer) after this one:

AFAICT all API usage of rr.ReposByArchName() sets includeTagged=False so I would like to drop that argument
rr.DistroHasRepos() should either become a pure bool/error return not the full repos (only weldr uses it currently)or we just use ReposByArchName()
Renaming ReposByArchName to something like ReposByDistroArchExcludeTagged or something descriptive

All of the above of course is more controversial so happy to discuss

While working in the area two tiny style/comment tweaks: 1. document that rpmmd.repository is the on-disk json which is diffrent from the parsed rpmmd.RepoConfig json which is relevant when writing e.g. json test cases (as in ibli#360) 2. switch `repositories := []rpmmd.RepoConfig{}` to just `var repositories []rpmmd.RepoConfig` which is `nil` by default which feels slightly more aligned with the usual go practise for empty lists. None of this is particularly important and we could easily drop this if undesired.

We have some ReposBy*() helpers already on RepoRegistry. So make things conistent by renaming the existing DistroHasRepos() to reposByDistroArch() as it behaves the same way as the others so the naming should also be consistent. Then create a compat wrapper for the old DistroHasRepos(). In a followup we should either remove this compat wrapper (the only consumer is weldr.go) or at least make it return eiher a bool or an error, its weird that we have a "hasFoo" that returns the value too.

This commit fixes the `TestInvalidReposByArch` which was not actually testing much because with the existing code it would error with: ``` failed to parse distro ID string: error when parsing distro name... ``` There was also a copy/paste mistake in 93c6336 which resulted in the code testing the same code-path twice (i.e. both times the "include-tagged" was set to false). The test now tests for the real error message.

By using `ReposByImageTypeName()` we can drop the custom `filterRepos()` which re-implements what ReposByImageTypeName() already does.

thozza

LGTM. I'd suggest actually removing DistroHasRepos() in Weldr if a different function can replace it.

thozza · 2025-10-31T07:32:20Z

pkg/reporegistry/reporegistry.go

+// (deprecated) DistroHasRepos is just here for compatbility with weldr which could
+// use `ReposByArchName(distro, arch, true)` instead.
+func (r *RepoRegistry) DistroHasRepos(distro, arch string) ([]rpmmd.RepoConfig, error) {
+	return r.reposByDistroArch(distro, arch)
+}
+


So why keep it and not fix the Weldr API to use ReposByArchName()?

Excellent point, I should have linked tohttps://github.com/osbuild/osbuild-composer/pull/4910 - sorry for that. Once that is in I can drop this compat wrapper.

supakeen

Thanks, lets do what @thozza says and drop the last function from weldr as well.

thozza · 2025-10-31T08:04:26Z

...

AFAICT all API usage of rr.ReposByArchName() sets includeTagged=False so I would like to drop that argument

This should be OK, since all tags in the repos are for image types. GCP is the only image type that uses this in our "official" repo configs, and I would call it an implementation detail that the user does not need to know about. IOW, if the user wants to install any packages from the GCP repositories, it is sensible to require that these repositories are also explicitly defined by the user.

rr.DistroHasRepos() should either become a pure bool/error return not the full repos (only weldr uses it currently)or we just use ReposByArchName()

I don't have a strong preference. The solution should be based on what Weldr API uses, rather than the function name. That said, I'm all for making things simpler and nicer.

Renaming ReposByArchName to something like ReposByDistroArchExcludeTagged or something descriptive

Personally, I'm OK with ReposByArchName() and documenting the behavior in the doc text. If you really want to rename it to something that expresses that tagged repos are not included, I'd suggest something shorter, such as CommonReposByArchName or even CommonReposByArch.

Changes with 0.210.0 ---------------- - Make bootupd metadata generation configurable per distro (osbuild/images#1958) - Author: Paul Whalen, Reviewers: Lukáš Zapletal, Simon de Vlieger - many: add support for bootc container based anaconda ISOs (HMS-9495) (osbuild/images#1906) - Author: Michael Vogt, Reviewers: Achilleas Koutsou, Simon de Vlieger, Tomáš Hozza - osbuild/osbuild-exec: make osbuild command a variable (osbuild/images#1965) - Author: Sanne Raymaekers, Reviewers: Lukáš Zapletal, Simon de Vlieger Changes with 0.211.0 ---------------- - Update osbuild dependency commit ID to latest (osbuild/images#1968) - Author: SchutzBot, Reviewers: Achilleas Koutsou, Simon de Vlieger - Update snapshots to 20251023 (osbuild/images#1967) - Author: SchutzBot, Reviewers: Achilleas Koutsou, Simon de Vlieger - deps: switch yaml libraries (osbuild/images#1970) - Author: Simon de Vlieger, Reviewers: Achilleas Koutsou, Michael Vogt - osbuild/monitor: increase status scanner buffer size to 16MiB (osbuild/images#1977) - Author: Sanne Raymaekers, Reviewers: Michael Vogt, Simon de Vlieger - osbuild: tweak how we can mock osbuild slightly (osbuild/images#1969) - Author: Michael Vogt, Reviewers: Achilleas Koutsou, Sanne Raymaekers - schutzbot/terraform: bump terraform sha (osbuild/images#1976) - Author: Florian Schüller, Reviewers: Achilleas Koutsou, Sanne Raymaekers, Simon de Vlieger Changes with 0.212.0 ---------------- - distro,manifest: have a single `LoraxTemplates` type (osbuild/images#1975) - Author: Michael Vogt, Reviewers: Achilleas Koutsou, Simon de Vlieger - distro: cleanup/reuse YAML for bootc ISO (osbuild/images#1971) - Author: Michael Vogt, Reviewers: Lukáš Zapletal, Simon de Vlieger - distro: cleanup/reuse YAML for bootc distro (osbuild/images#1944) - Author: Michael Vogt, Reviewers: Achilleas Koutsou, Simon de Vlieger - distro: load distros from multiple files (osbuild/images#1974) - Author: Simon de Vlieger, Reviewers: Michael Vogt, Tomáš Hozza - osbuild: switch from a bufio.Scanner to a bufio.Reader (osbuild/images#1978) - Author: Michael Vogt, Reviewers: Lukáš Zapletal, Simon de Vlieger - reporegistry: various cleanups (osbuild/images#1980) - Author: Michael Vogt, Reviewers: Simon de Vlieger, Tomáš Hozza - various: fix `make gh-action-test` and update README (osbuild/images#1981) - Author: Michael Vogt, Reviewers: Lukáš Zapletal, Simon de Vlieger Changes with 0.213.0 ---------------- - data: update fedora 43 repositories (osbuild/images#1983) - Author: Simon de Vlieger, Reviewers: Michael Vogt, Sanne Raymaekers - osbuild-exec: add buildlog and monitor support (osbuild/images#1966) - Author: Sanne Raymaekers, Reviewers: Michael Vogt, Simon de Vlieger

mvo5 added 4 commits October 30, 2025 10:15

gen-manifests: use ReposByImageTypeName()

3e3a86e

By using `ReposByImageTypeName()` we can drop the custom `filterRepos()` which re-implements what ReposByImageTypeName() already does.

mvo5 requested a review from a team as a code owner October 30, 2025 10:51

mvo5 requested review from achilleas-k, croissanne and supakeen October 30, 2025 10:51

mvo5 mentioned this pull request Oct 30, 2025

weldr,cloudapi: tweaks around the reporegistry handling osbuild/osbuild-composer#4910

Merged

thozza approved these changes Oct 31, 2025

View reviewed changes

supakeen approved these changes Oct 31, 2025

View reviewed changes

supakeen added this pull request to the merge queue Oct 31, 2025

Merged via the queue into osbuild:main with commit 8eef048 Oct 31, 2025
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

reporegistry: various cleanups #1980

reporegistry: various cleanups #1980

Uh oh!

mvo5 commented Oct 30, 2025

Uh oh!

thozza left a comment

Uh oh!

thozza Oct 31, 2025

Uh oh!

mvo5 Oct 31, 2025

Uh oh!

supakeen left a comment

Uh oh!

thozza commented Oct 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

reporegistry: various cleanups #1980

reporegistry: various cleanups #1980

Uh oh!

Conversation

mvo5 commented Oct 30, 2025

Uh oh!

thozza left a comment

Choose a reason for hiding this comment

Uh oh!

thozza Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

mvo5 Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

supakeen left a comment

Choose a reason for hiding this comment

Uh oh!

thozza commented Oct 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants