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 support for downloadonly in DNF5 #559

Merged
merged 3 commits into from
May 29, 2023

Conversation

j-mracek
Copy link
Contributor

@j-mracek j-mracek commented May 24, 2023

Resolve: #537

j-mracek added 3 commits May 24, 2023 15:22
The downloadonly is important for several components.
The message clarify what approval of transaction will do.
@jan-kolarik
Copy link
Member

jan-kolarik commented May 25, 2023

The PR itself looks great, but I wanted to test the parameter in CLI and it is unknown. I think we are missing the owner.opt_binds().add("downloadonly", downloadonly); in the main config. Can you please fix that?

EDIT: Tested again and it looks OK 🙂

dnf5/main.cpp Show resolved Hide resolved
@jrohel
Copy link
Contributor

jrohel commented May 26, 2023

@jan-kolarik

I think we are missing the owner.opt_binds().add("downloadonly", downloadonly); in the main config. Can you please fix that?

I don't think so. As you can see in the source code, downloadonly is a "runtime only option".
OptionBool downloadonly{false}; // runtime only option

And from my point of view, downloadonly is a specific option of dnf5 application. It is not a libdnf(5) option. Do we want to add this option to the "dnf.conf " file?
The fact that the application option is listed in libdnf (with the note "runtime only option") is strange. Probably from the time when we needed to somehow maintain options between C and Python code. I still tend to separate libdnf library configuration options (applied to each user of the library) and application dnf5 configuration options, which may not be of interest to another application.

@jan-kolarik
Copy link
Member

@jan-kolarik

I think we are missing the owner.opt_binds().add("downloadonly", downloadonly); in the main config. Can you please fix that?

I don't think so. As you can see in the source code, downloadonly is a "runtime only option". OptionBool downloadonly{false}; // runtime only option

And from my point of view, downloadonly is a specific option of dnf5 application. It is not a libdnf(5) option. Do we want to add this option to the "dnf.conf " file?

I see. I was confused that the --downloadonly parameter was not recognized for me when testing the PR and I just noticed the binding is missing and it might be the cause. Anyway, I tried to pull the PR again today and everything seems working correctly, so it was just probably my broken local setup.

@j-mracek
Copy link
Contributor Author

j-mracek commented May 29, 2023

@jan-kolarik

I think we are missing the owner.opt_binds().add("downloadonly", downloadonly); in the main config. Can you please fix that?

I don't think so. As you can see in the source code, downloadonly is a "runtime only option". OptionBool downloadonly{false}; // runtime only option
And from my point of view, downloadonly is a specific option of dnf5 application. It is not a libdnf(5) option. Do we want to add this option to the "dnf.conf " file?

I see. I was confused that the --downloadonly parameter was not recognized for me when testing the PR and I just noticed the binding is missing and it might be the cause. Anyway, I tried to pull the PR again today and everything seems working correctly, so it was just probably my broken local setup.

In the first version I enabled binding to allow --setopt=downloadonly=true but I was not sure whether we want to support that. Be honest I did not found any reason to not add it also.

I think that a binding of the option could be added together with implementation of #555.

@jan-kolarik jan-kolarik added this pull request to the merge queue May 29, 2023
Merged via the queue into rpm-software-management:main with commit aa70bf2 May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Please implement update --downloadonly
3 participants