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

Add priority option for repo configuration #84

Merged
merged 6 commits into from
Jan 17, 2025
Merged

Conversation

kellyma2
Copy link
Collaborator

@kellyma2 kellyma2 commented Jan 3, 2025

Currently there's no user-specified ordering for the defined repositories and because of the way the code works it will (counter intuitively) choose RPMs from repos later in the list.

This change introduces an explicit repo priority so that users can force ordering of repos to choose RPMs from.

Currently there's no user-specified ordering for the defined
repositories and because of the way the code works it will (counter
intuitively) choose RPMs from repos later in the list.

This change introduces an explicit repo priority so that users can
force ordering of repos to choose RPMs from.
Copy link
Owner

@rmohr rmohr left a comment

Choose a reason for hiding this comment

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

lgtm over all, do you think you could add a small unit test?

Now that we have the loading code exposed in a way that is a bit more
manageable we can add some tests around it.

We also add a set of helpers in a separate source file which will be
used by tests for the reducer itself.
This change approximates the reducer operations and provides tests for
them.  It leverages the existing helpers produced for the loader_test
to reduce the repetitiveness of constructing the test inputs.
The first iteration of this change produced a scenario where we'd drop
packages that have a different version which we previously were
retaining.  This change fixes that and includes differently versioned
packages while accounting for the repository priority otherwise.
Copy link
Collaborator

@manuelnaranjo manuelnaranjo 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 this together with my stable solver setup will give the results we expect.
Let's wait a few days see if @rmohr has some time to look at it, otherwise I'll approve

@rmohr
Copy link
Owner

rmohr commented Jan 17, 2025

Currently there's no user-specified ordering for the defined repositories and because of the way the code works it will (counter intuitively) choose RPMs from repos later in the list.

Order makes sense, I just want to add that it should eventually use the newest versions matching a constraint, but yes if the same versions are shipped by two repos it just uses one of those kind of arbitrary.

@kellyma2 kellyma2 merged commit daae7c2 into rmohr:main Jan 17, 2025
13 checks passed
@kellyma2 kellyma2 deleted the priority branch January 17, 2025 19:30
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.

3 participants