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

Additional features for config-manager #1128

Merged
merged 5 commits into from
Mar 8, 2024

Conversation

jrohel
Copy link
Contributor

@jrohel jrohel commented Jan 4, 2024

It solves tasks in the #1011 issue except for documentation and does not replace crc32 with library functions.

Requires: rpm-software-management/ci-dnf-stack#1429 adjustment of CI tests.

Closes: #1011 .

@pkratoch
Copy link
Contributor

I am also wondering if the messages should be only written to logs or if there also shouldn't be a warning on stderr.

@pkratoch pkratoch removed a link to an issue Jan 10, 2024
8 tasks
@pkratoch
Copy link
Contributor

I unlinked the issue #1011 , so that it's not closed upon merging this, because it wouldn't be fully resolved. We could either split the issue and link the newly created one or leave it as it is and then manually mark the individual items as resolved.

bool changed = false;
parser.read(repos_override_file_path);
if (!in_repos_opts_to_remove.empty()) {
const auto & repos_override_file_path = get_config_manager_repos_override_file_path(config);
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 not a problem of this PR, but I encountered this:

When I run dnf5 config-manager unsetopt fedora.gpgcheck (or any other option), there is no output, but nothing gets changed. Only when I finally looked at logs, I noticed there is config-manager: Request to remove repository option but config file not found: /etc/dnf/repos.override.d/99-config_manager.repo.

Shouldn't this file get created if it doesn't exist yet? Or am I missing something?

Moreover, this makes me think even more that the warnings should get also on the stderr, because it took me some time to figure out why nothing is happening.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pkratoch

Shouldn't this file get created if it doesn't exist yet?

No. The file contains overrides of repository configuration. A non-existent file has the same meaning as an empty file - no overrides are defined.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, right. It doesn't make sense to create the file for unsetting the options. I was confused only because I initially expected the original repo file to be modified and seemingly nothing happened when I run the unsetopt command. But now I see it only works over the overrides and the error message makes that clear.

@jrohel
Copy link
Contributor Author

jrohel commented Feb 26, 2024

I wanted to unify logging to the logger and to the terminal. It is not yet implemented. So I will modify this PR to directly print the warning to stderr.

@jan-kolarik
Copy link
Member

I unlinked the issue #1011 , so that it's not closed upon merging this, because it wouldn't be fully resolved. We could either split the issue and link the newly created one or leave it as it is and then manually mark the individual items as resolved.

I relinked it again as I created separate issues for the remaining tasks and mentioned them in the original issue.

@pkratoch pkratoch added this pull request to the merge queue Mar 8, 2024
Merged via the queue into rpm-software-management:main with commit e3deda9 Mar 8, 2024
9 of 22 checks passed
@rffontenelle
Copy link
Contributor

Translation-related doubt: "repository" and "main" in the warnings below should be kept as in English (e.g. are they keywords in the code or command-line option), or they can be translated?

config-manager: Request to remove repository option but repoid is not present in the overrides: {}

config-manager: Request to remove unsupported main option: {}

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.

Additional features for config-manager
4 participants