Add multi-resolve functions to resolvers#2072
Conversation
8150679 to
26d0115
Compare
|
Note that full (resolved) manifests have changed because the order of containers in the sources changes when using a different resolver. This isn't a functional change and doesn't affect the manifest ID. |
This comment was marked as outdated.
This comment was marked as outdated.
26d0115 to
9a9706a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
A convenience function that runs Depsolve() on each package set slice and returns a map of results with the same keys. This is a follow-up to 5abe1d5 which introduced the DefaultDepsolver() function in manifestgen. It essentially moves the core of that function into the dnfjson package.
9a9706a to
150b112
Compare
thozza
left a comment
There was a problem hiding this comment.
LGTM.
I would not spend too much time optimizing or discussing SBOMs in the Solver, because at the end of the day, Solver won't be the place that will handle SBOMs in the future. All the SBOM code will be eventually removed from the Solver.
pkg/depsolvednf/depsolvednf_test.go
Outdated
| solver := NewSolver("platform:el9", "9", "x86_64", "rhel9.0", tmpdir) | ||
| solver.SBOMType(tc.sbomType) |
There was a problem hiding this comment.
I would love us to move away from this pattern of constructing an instance just to modify it in the next step. Ideally moving towards the functional options pattern.
There was a problem hiding this comment.
Yeah the functional options you showed me the other day looks really nice.
Add a new property to the solver for the SBOM type. This simplifies the signatures of DepsolveAll(). Ideally we would make the same change for Depsolve(), but that would be a breaking API change and we don't want to be too disruptive now.
Move manifestgen.DefaultCommitResolver() to ostree.ResolveAll(). The convenience function in manifestgen only calls the resolve function in a nested loop, so let's make it part of the ostree package where the original resolver and all ostree related types and functions are defined.
Extend the Resolver interface to implement Resolve() and ResolveAll(). This is consistent with the API of the other resolvers. The blockingResolver implements Resolve() as its main resolver function. The Add() function now wraps Resolve() and collects the resolved specs in the results slice. This is the inverse of the asyncResolver where Resolve() wraps Add(). ResolveAll() calls Resolve() iteratively on all container source specs.
Add a 1 minute timeout to each resolve call.
150b112 to
d115985
Compare
lzap
left a comment
There was a problem hiding this comment.
I would really like us utilizing context more, while it is fine adding more fire-grained timeouts you could have an overall deadline as well. Also, contexts are great for logging contexts, granted, this is more relevant for the service (per request, per org) than CLI but it has its own use there as well.
Anyways, all is fine.
This PR adds convenience functions to the package, container, and ostree resolvers. This is an old branch I had sitting around that was following up from the original introduction of the
manifestgenpackage. While working on Add proxy support to image-builder-cli (HMS-9645), I remembered that I wanted to move some of the conveniences ofmanifestgeninto the resolver packages themselves and this is part of that.Some background
The general flow of resolving content to generate a manifest is:
In (simplified) code, this looks like:
NOTE: This isn't entirely accurate code, but is simplified to emphasise the important parts (e.g. error return values are dropped).
The
manifestgen.Generator.Generatefunction implements this flow, using the default configurations for each resolver and allows overriding them when needed.New stuff
This PR only provides the
ResolveAll()convenience which performs the iteration over the unresolved content indexed by the pipeline names. This moves some of the convenience provided bymanifestgen.Generator.Generate()into the resolvers themselves, making them useful in cases wheremanifestgenisn't practical (e.g. in osbuild-composer where content is resolved on different workers).Container resolvers
For a while now, we've had two container resolver implementations, an async one (the original) and a blocking resolver (introduced in #1176). There was a short discussion on that PR about the differences between the two. I summarised the consequences of switching to a blocking resolver in this comment. I'd like to prioritise consistency here over potential performance, but if we think we would benefit from parallel container resolution, we could go back to only having the async resolver but make its interface the same as the other resolvers (with
Resolve()andResolveAll()).