cmd/build: use the new manifestgen package #1165
Conversation
pkg/manifestgen/manifestgen.go
Outdated
| // CustomRepos overrides the default repository selection. | ||
| // This is mostly useful for testing | ||
| CustomRepos []rpmmd.RepoConfig |
There was a problem hiding this comment.
I have a bit of a nitpick related to the name of the variable. "custom repos" already has the connotation of being provided by the user. We sometimes call it "3rd party repos". See https://osbuild.org/docs/user-guide/repository-customizations/.
In any case, I'd suggest to use something like OverrideRepos and keep the "custom repos" for later use cases, such as supporting user-defined repositories.
Moreover, do you see this functionality being used in the future for use cases such as within build system pipelines to produce images from a new compose?
There was a problem hiding this comment.
I renamed the field now, I agree that there is a risk of confusion here.
I personally would not mind to drop this, it is needed if we want to port "build" to fully use manifestgen but as visible in "build" it is not easy. I think @achilleas-k mentioned at some point he like the ability to point to a single repo file for testing.
I guess we can:
a) drop it and leave images as is (using parts of manifestgen only)
b) include it
I don't strongly mind either way.
There was a problem hiding this comment.
Well, this depends on the answer to my question in the original comment - will this be the way to explicitly define repos to use for build / depsolving / manifest-gen when using tools built on this functionality (ibcli), when being used as part of a CI/CD pipeline or distro build system? I suspect that "probably yes". So, from that point of view, it makes sense to have a way to define repos without using a repo registry at all.
2aa1671 to
1c6b4c5
Compare
This commit adds support to override the manifest generator with user-specified repositories. This is useful for e.g. the `cmd/build` command that has the ability to override the repostiory information via an arbitrary file.
This commit adds a new `manifestgen.Options.WarningsOutput` io.Writer that can be used to make warnings visible. Without the WarningsOutput option any warnings will generate an errror.
This commit moves the `cmd/build` command to the new manifestgen package to the full extend. Some of the output will slightly change now but this commit tries to keep the errors/messages as similar as possible.
1c6b4c5 to
8d70047
Compare
| @@ -89,6 +94,7 @@ func New(reporegistry *reporegistry.RepoRegistry, opts *Options) (*Generator, er | |||
| rpmDownloader: opts.RpmDownloader, | |||
| sbomWriter: opts.SBOMWriter, | |||
| customSeed: opts.CustomSeed, | |||
| overrideRepos: opts.OverrideRepos, | |||
There was a problem hiding this comment.
Nitpick: would it make sense to return an error in case opts.OverrideRepos and reporegistry are both provided?, Because in such case, the reporegistry will always be ignored. Similarly, would an error make sense if none is provided?
Note that I could also create a PR for the first two commits that extend
manifestgen and then do the build.go changes separately (or not
at all if that is preferred). The first two commits will help ibcli as well,
once those are in we can add
--ignore-warnings(or something)like this to "image-builder-cli" and also a "--override-repository"
option that works differently from "datadir" (just like we do here).
There is a question in the PR about how to best deal with repos
on the API level (which will affect how this will be done in ibcli).
Individual commits below:
This commit moves the
cmd/buildcommand to the new manifestgenpackage to the full extend. Some of the output will slightly
change now but this commit tries to keep the errors/messages
as similar as possible.
manifestgen: add new
manifestgen.Options.WarningsOutputThis commit adds a new
manifestgen.Options.WarningsOutputio.Writer that can be used to make warnings visible. Without
the WarningsOutput option any warnings will generate an errror.
manifestgen: add support for custom repos override
This commit adds support to override the manifest generator with
custom repositories. This is useful for e.g. the
cmd/buildcommand that has the ability to override the repostiory information
via a custom file.