voxpupuli / puppet-systemd

Puppet module to manage systemd
https://forge.puppet.com/puppet/systemd
Apache License 2.0
53 stars 142 forks source link

Inconsistent values for `notify_service` in replacement for `systemd::service_limits` #481

Open johnwarburton opened 1 month ago

johnwarburton commented 1 month ago

Affected Puppet, Ruby, OS and module versions/distributions

How to reproduce (e.g Puppet code you use)

The deprecation notice states:

The type systemd::service_limits is deprecated and systemd::manage_dropin or systemd::dropin_file should be used instead.

What are you seeing

Wanting to set LimitNOFILE with the puppet-systemd module, I read the deprecation notice and used systemd::dropin_file as we already have a working example. The limit was set correctly for the service, but on re-review of the module code, I felt using systemd::manage_dropin would be less error prone

When using service_entry in systemd::manage_dropin, the dropin file was correctly created but the service did not have its limit changed. I had to add notify_service: true for the service to be restarted and set the limit correctly

What behaviour did you expect instead

Given the deprecation notice and systemd::manage_dropin or systemd::dropin_file are offered as equal alternatives, I would expect both types to have the same defaults for notify_service, whereas we have different defaults

$ egrep -r 'Boolean.*notify_service' manifests/
manifests/manage_dropin.pp:  Boolean                          $notify_service          = false,
manifests/dropin_file.pp:  Boolean                                     $notify_service          = true,

Any additional information you'd like to impart

I am guessing some of this came from the work in https://github.com/voxpupuli/puppet-systemd/issues/463 Also, I am well aware I could be told to RTFM, which I ended up doing, but in the spirit of the principle of least surprise and people who come after me, it would be nice to have a consistent default for notify_service (and IMHO, that would be true)

Thanks for such a great module, always appreciate your work

smortex commented 1 month ago

+1 for consistent behavior. IMHO, the default should be to restart what has to be restarted, and offer the possibility to opt-out from restarting services if needed be.