pkg/depsolvednf: decouple Solver methods from API wire-format implementation to prepare for v2 Solver API (HMS-9062) #2068
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
While working on the osbuild-depsolve-dnf parts, I realized that the `BaseURL` information in this case comes from the repository metadata, where it is almost never set. So in reality, the value would be almost always empty. The baseurl value is and should be deduced from the repository config that the package comes from. Signed-off-by: Tomáš Hozza <[email protected]>
Add the NVR() helper. This property is used by the depsolvednf package to sort list of packages, so let's implement it in a single place. Signed-off-by: Tomáš Hozza <[email protected]>
The unit test verifies that the expected number of packages are in the response and that the slice is sorted by package's NVR string. Signed-off-by: Tomáš Hozza <[email protected]>
The unit test verifies that the expected list of packages is returned for certain scenarios and that the list is sorted by package's NVR string. Signed-off-by: Tomáš Hozza <[email protected]>
The function is not used anywhere as far as I can tell. There is an unexported version of the error parser, which is called by the `run()` function. Signed-off-by: Tomáš Hozza <[email protected]>
This moves a common and important validation logic from makeDepsolveRequest() a level higher, before the depsolve request is constructed. The goal is to keep only API wire-format specific code in the makeDepsolveRequest() to make it easy to iterate on API versions. Signed-off-by: Tomáš Hozza <[email protected]>
thozza
force-pushed
the
HMS-9062-refactoring-and-tests
branch
from
865468c to
04c4767
Compare
The goal is to later make reposFromRPMMD() specific to the API implementation and not being a method of the Solver. This will make the transition easier, because there would be less data that need to be passed to the function that transforms the rpmmd.RepoConfig to the wire-format. Signed-off-by: Tomáš Hozza <[email protected]>
Extract the error parsing from the Solver request wire-format. This will make adding a new API version easier, since no more changes would be needed to adjust error parsing. Also change the run() function to not return parsed error, but instead return and error with the raw error message. This moves the task to parser the actual error to the caller, which makes more sense since the run() function should not care about the wire-format at all. Signed-off-by: Tomáš Hozza <[email protected]>
The `rhsmMap` was previously constructed in `makeDepsolveRequest()`, but not used at all and returned to the caller. The caller then used it to interpret the Solver API response. Considering this, the makeDepsolveRequest() is simply the wrong place to construct `rhsmMap`. Move the construction of the `rhsmMap` to the caller. This decouples the request construction even more from the Solver implementation. This will make decoupling the wire-format construction from the Solver implementation much cleaner. Signed-off-by: Tomáš Hozza <[email protected]>
Make optionalMetadataForDistro() a standalone function that takes modulePlatformID as parameter. This will eventually enable API handler implementation use it without Solver access. Signed-off-by: Tomáš Hozza <[email protected]>
These will wrap the Solver command response in a similar way as DepsolveResult does now for the 'depsolve' command. The structs are not used yet. They will be used by the future API handler interface. Signed-off-by: Tomáš Hozza <[email protected]>
The apiHandler describes methods that an osbuild-depsolve-dnf API handler must implement. The code will be refactored to use the interface later, which will make adding new API handlers much simpler thanks to clear separation of responsibilities. Signed-off-by: Tomáš Hozza <[email protected]>
Add a version of the current (v1) API implementation, which implements the apiHandler interface. The implementation is not used yet. It will be swapped with the current implementation that is coupled with the Solver, in the following commits. The goal is to keep the API as is, without any noticeable change. Add unit tests for make*Request() methods, verifying that the produced JSON is as expected. The parse*Result() will be tested indirectly by the unit tests that verify the Solver methods, when they switch to use API handler implementation. Signed-off-by: Tomáš Hozza <[email protected]>
Signed-off-by: Tomáš Hozza <[email protected]>
Add a helper method for the Solver, which returns the solverConfig instance from the current Solver state. The return value will be then passed to the API handler methods. Signed-off-by: Tomáš Hozza <[email protected]>
This decouples the run() function from the API wire-format struct implementation. Now it takes raw request as []byte and returns raw response as []byte. The caller is responsible for (un)marshaling the response/request from and to JSON. Signed-off-by: Tomáš Hozza <[email protected]>
The caching for the dump and search commands depended on the API request wire-format struct. To not need to complicate the API handler interface with this specific aspect, replace the old way of calculating hash by hashing the whole raw request. Although the new implementation will produce different hashes than the old one, this is a one time cache invalidation. The functionality is technically the same. Signed-off-by: Tomáš Hozza <[email protected]>
Modify Solver.Depsolve() to use activeHandler instead. Remove methods that are no longer needed. Signed-off-by: Tomáš Hozza <[email protected]>
Modify Solver.FetchMetadata() to use activeHandler instead. Remove methods that are no longer needed. Signed-off-by: Tomáš Hozza <[email protected]>
Modify Solver.SearchMetadata() to use activeHandler instead. Remove methods and structs that are no longer needed. Signed-off-by: Tomáš Hozza <[email protected]>
Remove the remaining old types and methods that are no longer used anywhere after migrating all Solver methods to use activeHandler. Signed-off-by: Tomáš Hozza <[email protected]>
Extract the RHSM secrets override logic from Depsolve() into a separate applyRHSMSecrets() function. This logic overrides Package.Secrets from "org.osbuild.mtls" to "org.osbuild.rhsm" for packages coming from repositories with RHSM=true. This logic was previously untested. Signed-off-by: Tomáš Hozza <[email protected]>
thozza
force-pushed
the
HMS-9062-refactoring-and-tests
branch
from
04c4767 to
b37aa65
Compare
achilleas-k
approved these changes
Dec 5, 2025
Member
achilleas-k
left a comment
achilleas-k
left a comment
There was a problem hiding this comment.
Wow, that's a lot! Very clean PR, couldn't find anything to complain about.
LGTM
Comment on lines
+580
to
+582
| // optionalMetadataForDistro returns optional repository metadata types | ||
| // that should be downloaded for the given distro. | ||
| func optionalMetadataForDistro(modulePlatformID string) []string { |
Member
There was a problem hiding this comment.
Could drop all mentions of Fedora here now.
Member
Author
There was a problem hiding this comment.
Nice catch. I'll make this change in a follow-up.
lzap
approved these changes
Dec 5, 2025
Contributor
lzap
left a comment
lzap
left a comment
There was a problem hiding this comment.
Wow, 22 commits and I was able to find just one nit pick :-D
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Member
|
Lots of rebuilds happening on the merge queue. I wonder if it's a manifest generation or caching issue. Looking into it. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Member
|
Let's get #2075 merged first to stop the rebuilds on the merge queue. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This PR refactors the
pkg/depsolvednfpackage to introduce anapiHandlerinterface that abstracts the osbuild-depsolve-dnf API implementation. The goal is to enable easier iteration on Solver API versions by cleanly separating API wire-format handling (request building and response parsing) from the core Solver logic. The current v1 API has been extracted into a dedicatedv1Handlerimplementation. This is preparatory work for adding a v2 Solver API in a follow-up PR.Architectural Changes
The key design decision is the introduction of the
apiHandlerinterface that defines three pairs of methods for Request building and Response parsing.A
solverConfigstruct is used to pass solver configuration to handlers without coupling them to theSolvertype directly. Therun()function now accepts raw[]byterequest data instead of a typed*Requeststruct, making it agnostic to the API version being used.This architecture allows new API versions to be added by simply implementing the
apiHandlerinterface, without modifying the Solver methods themselves.Key Changes
apiHandlerinterface (api.go): Defines the contract for API version handlers with methods for building requests and parsing responsesv1Handlerimplementation (v1.go): Extracts all v1 API structs, request building, and response parsing logic from the main fileDepsolve(),FetchMetadata(), andSearchMetadata()now useactiveHandlerinstead of inline API logicrun()function: Now accepts raw[]byteinstead of a typed request structRequest.Hash()withhashRequest(): Caching now uses SHA256 of raw request bytes instead of custom field-based hashingapplyRHSMSecrets()helper: The RHSM secrets override logic is now a separate, testable functionBreaking Changes / Backward Compatibility
This PR is fully backward compatible. The external API surface of the
depsolvednfpackage remains unchanged. Internal types have been moved/removed, but these were not exported. The cache hash computation has changed, which will cause a one-time cache invalidation for existing caches.Testing
TestV1HandlerMakeDepsolveRequest,TestV1HandlerMakeDumpRequest,TestV1HandlerMakeSearchRequest) verifying JSON output matches expected formatSolver.FetchMetadata()andSolver.SearchMetadata()to ensure the refactoring doesn't break existing behaviorapplyRHSMSecrets()function covering various RHSM/mTLS/plain repo combinations