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

automatic: Fix documentation and ship config file #1476

Merged
merged 5 commits into from
May 15, 2024

Conversation

m-blaha
Copy link
Member

@m-blaha m-blaha commented May 9, 2024

  • changes the config files reading behavior - /usr/share/dnf5/dnf5-plugins/automatic.conf is always read to set the defaults, /etc/dnf/dnf5-plugins/automatic.conf is then used for host-specific overrides
  • fixes errors in the documentation
  • enhances documentation of changes between dnf4 and dnf5
  • ship default configuration file in /usr/share/dnf5/dnf5-plugins/automatic.conf

Resolves: #1289
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2279257
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2279258

Comment on lines 3 to 4
# copy this file to /etc/dnf/dnf5-plugins/automatic.conf and make your
# changes there.

Choose a reason for hiding this comment

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

Is a full copy really necessary? With that /usr vs. /etc schema, it's usually nicer to have a file in /etc/ which merely overrides some default keys. I.e. if /etc exists, will it still read the other key defaults from /usr?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not change (for now) the logic how the config files are read, merely added a distribution config file to the package. So currently the /usr config file is not being read if the /etc one exists.
But I get your point. Let me think about it till tomorrow. Meanwhile I added a blocked label to the PR.

Choose a reason for hiding this comment

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

Thanks @m-blaha FTR, no reason to block this PR, these are good changes. I was just wondering how it works. E.g. is it legit to create an /etc config with just a single key? If I set only apply_updates = yes in /etc, how will dnf-automatic treat e.g. the value of upgrade_type? As it works right now without any config file, I guess it has some builtin defaults anyway -- and hopefully they match the default file in /usr 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, /usr config file just contains builtin defaults - thus it's perfectly OK to include just a single config option into /etc config file. But you are right that it's not very intuitive - user kind of expects that /usr is used as a baseline and then /etc overrides are applied on top of it.

Choose a reason for hiding this comment

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

Thanks -- if the builtin defaults match the ones in /usr, that seems fine then. This would only mean that you can't change the one in /usr (ugh, never do that) and expect it to apply if you also have an /etc/ fiile. So this seems good enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did the change anyway (hopefully you don't mind the rebase). It seems more intuitive for the user that both config files are applied. It also enables us to adjust the default configuration in /usr if needed. Now everything should be in place and PR is ready for review.

Choose a reason for hiding this comment

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

Very nice, also good fix for the random delay!

(FTR: rebases are how git works. I never understood some people's objections against it)

@m-blaha m-blaha added the blocked Further work on issue or PR is blocked by something else label May 9, 2024
@m-blaha m-blaha removed the blocked Further work on issue or PR is blocked by something else label May 10, 2024
Copy link

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

@jan-kolarik jan-kolarik self-assigned this May 13, 2024
@jan-kolarik
Copy link
Member

jan-kolarik commented May 14, 2024

/packit test –identifier dnf5-tests

@jan-kolarik
Copy link
Member

Hmm, the tests don't look good, maybe a rebase is needed...

@m-blaha
Copy link
Member Author

m-blaha commented May 14, 2024

I can rebase it. But those failures are strange - completely unrelated to automatic.

The value of random_sleep was applied before the config files were read,
thus user was not able to alter its value.
Previously, only one config file was read - the first existing file from
either /etc/dnf/dnf5-plugins/automatic.conf or
/usr/share/dnf5/dnf5-plugins/automatic.conf.

Now, the config file reading behavior has been adjusted to meet user
expectations. Both files (if they exist) are read, with values from /usr
being applied first to serve as the baseline configuration. Values from
/etc are then applied as overrides.
- use the correct locations of the configuration files
- remove mentions of not any more supported systemd units
- consistently use "dnf5-automatic" or "dnf5 automatic" respecively
@m-blaha
Copy link
Member Author

m-blaha commented May 14, 2024

Rebased. Let's wait for results...

@jan-kolarik
Copy link
Member

I can rebase it. But those failures are strange - completely unrelated to automatic.

Yes, most probably they are, but to be sure... Thanks!

@jan-kolarik jan-kolarik added this pull request to the merge queue May 15, 2024
Merged via the queue into main with commit b84be9d May 15, 2024
18 of 19 checks passed
@jan-kolarik jan-kolarik deleted the mblaha/automatic-doc branch May 15, 2024 06:53
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.

Document changes in dnf5-plugin-automatic files between DNF 4 and 5
3 participants