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.

Sign up for GitHub

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

Jump to bottom

Add the possibility to load filelists conditionally #1635

Merged
merged 4 commits into from
Jan 25, 2024
Merged

Add the possibility to load filelists conditionally #1635

merged 4 commits into from
Jan 25, 2024
+31 −9

Conversation

jan-kolarik
Copy link
Member

@jan-kolarik jan-kolarik commented Nov 7, 2023 •
edited
Loading

Setup the Repo objects to allow conditional loading of the filelists metadata.

A new optional_metadata_types option was created to be used for configuring if filelists metadata should be downloaded by default.

Currently, the changes should not introduce any new behavior by default. The introduction of dropping the filelists loading by default should come after the system-wide change proposal is accepted.

Follow-up PR in dnf: rpm-software-management/dnf#2012.
Related: #1637.
Targetted for: https://issues.redhat.com/browse/RHEL-12355.

@jan-kolarik jan-kolarik mentioned this pull request Nov 7, 2023
Merged
@jan-kolarik jan-kolarik force-pushed the jkolarik/optional-filelists branch 2 times, most recently from ca59009 to 3318b63 Compare November 7, 2023 10:57
@j-mracek j-mracek self-assigned this Nov 8, 2023
This was referenced Dec 8, 2023
Merged
Merged
j-mracek
j-mracek reviewed Dec 12, 2023
View reviewed changes
libdnf/dnf-context.cpp Outdated Show resolved Hide resolved
@jan-kolarik jan-kolarik force-pushed the jkolarik/optional-filelists branch from 3318b63 to 45efc6d Compare January 12, 2024 09:44
@j-mracek
Copy link
Contributor

The last piece that is not covered is requires, conflict, and version.

I recommend to increase minor version +1. The same for DNF. I also recommend to ensure that the new libdnf cannot be installed with old version of DNF because when the default will change to FALSE then the old version of DNF stops to download filelists without possibility of override. A conflict in libdnf with dnf will handle that.

j-mracek
j-mracek reviewed Jan 15, 2024
View reviewed changes
libdnf/repo/Repo.hpp Outdated Show resolved Hide resolved
@jan-kolarik
Copy link
Member Author

The last piece that is not covered is requires, conflict, and version.

I recommend to increase minor version +1. The same for DNF. I also recommend to ensure that the new libdnf cannot be installed with old version of DNF because when the default will change to FALSE then the old version of DNF stops to download filelists without possibility of override. A conflict in libdnf with dnf will handle that.

Thanks for info. I was planning to implement this in a separate follow-up PR. This one is just mentioned to have the same functionality as until now, but prepare the internals for the easy switch to "enable_filelists = false;" by default.

@jan-kolarik jan-kolarik force-pushed the jkolarik/optional-filelists branch from 45efc6d to d9db6cd Compare January 16, 2024 11:26
@jan-kolarik jan-kolarik mentioned this pull request Jan 16, 2024
Open
@jan-kolarik jan-kolarik force-pushed the jkolarik/optional-filelists branch from d9db6cd to d8a3df1 Compare January 16, 2024 11:56
@jan-kolarik
repo: Load filelists based on the configuration
e588234 
Prepare libdnf to not load the filelists metadata by default in the future.
They will be loaded only when requested through the `optional_metadata_types`
configuration option.
@jan-kolarik
config: Add optional_metadata_types option
3cfe9d7 
A new optional_metadata_types option was created to be used for configuring if filelists metadata should be downloaded by default.
@jan-kolarik
dnf-context: Load default filelists value from config
edc343c 
Use the default value for enabling filelists metadata from main configuration. Override the value when explicitly using the setter.
@jan-kolarik jan-kolarik force-pushed the jkolarik/optional-filelists branch from d8a3df1 to edc343c Compare January 18, 2024 12:05
@jan-kolarik
Copy link
Member Author

@j-mracek I simplified the PR, so the decision to load filelists is only loaded from the optional_metadata_types configuration option and cannot be overridden per-repo to align with the current dnf5 implementation.

j-mracek
j-mracek reviewed Jan 19, 2024
View reviewed changes
libdnf/repo/Repo.cpp

auto & optionalMetadataTypes = conf->getMainConfig().optional_metadata_types().getValue();
auto loadFilelists = std::find(optionalMetadataTypes.begin(), optionalMetadataTypes.end(), "filelists") !=
optionalMetadataTypes.end();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great idea

@j-mracek
Copy link
Contributor

j-mracek commented Jan 19, 2024 •
edited by jan-kolarik
Loading

LGTM as well rpm-software-management/dnf#2012.

  • I only recommend/request to update spec in both projects:

libdnf.spec

%global libdnf_minor_version 73

I also have been thinking about %global dnf_conflict 4.19.0 but the current implementation is so simple and mainly in libdnf therefore I do not consider it as required.

dnf.spec

%global hawkey_version 0.73.0
Version:        4.19.0

  • This is a significant change therefore I strongly recommend to add CI tests (in this PR of create an issue with urgent priority)

  • (Optional) Additionally I would recommend to switch to Fedora 40 planned default in upstream but in separate commit and revert it in downstream for Fedora < 40. It does not require to be in this PR.

@jan-kolarik
Copy link
Member Author

jan-kolarik commented Jan 22, 2024 •
edited by janblazek
Loading

LGTM as well rpm-software-management/dnf#2012.

  • I only recommend/request to update spec in both projects:

libdnf.spec

%global libdnf_minor_version 73

I also have been thinking about %global dnf_conflict 4.19.0 but the current implementation is so simple and mainly in libdnf therefore I do not consider it as required.

dnf.spec

%global hawkey_version 0.73.0
Version:        4.19.0
  • This is a significant change therefore I strongly recommend to add CI tests (in this PR of create an issue with urgent priority)
  • (Optional) Additionally I would recommend to switch to Fedora 40 planned default in upstream but in separate commit and revert it in downstream for Fedora < 40. It does not require to be in this PR.

LGTM as well rpm-software-management/dnf#2012.

  • I only recommend/request to update spec in both projects:

libdnf.spec

%global libdnf_minor_version 73

I also have been thinking about %global dnf_conflict 4.19.0 but the current implementation is so simple and mainly in libdnf therefore I do not consider it as required.

dnf.spec

%global hawkey_version 0.73.0
Version:        4.19.0
  • This is a significant change therefore I strongly recommend to add CI tests (in this PR of create an issue with urgent priority)
  • (Optional) Additionally I would recommend to switch to Fedora 40 planned default in upstream but in separate commit and revert it in downstream for Fedora < 40. It does not require to be in this PR.

The first item has been delivered, and the other two will be addressed in the follow-up rpm-software-management/dnf#2038.

@jan-kolarik jan-kolarik force-pushed the jkolarik/optional-filelists branch from 7efb69a to 2986bdb Compare January 25, 2024 09:16
@j-mracek
Copy link
Contributor

LGTM

@j-mracek j-mracek merged commit 24c2a04 into dnf-4-master Jan 25, 2024
6 checks passed
@j-mracek j-mracek deleted the jkolarik/optional-filelists branch January 25, 2024 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@j-mracek j-mracek j-mracek left review comments

@jrohel jrohel jrohel left review comments

Assignees

@j-mracek j-mracek

Labels
None yet
Projects
DNF team
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jan-kolarik @j-mracek @jrohel