Code review comment for ~dashmage/charm-sysconfig:systemd-reboot-notification

Revision history for this message
Andrea Ieri (aieri) wrote (last edit ):

A few comments:

* setting config options at deploy time or subsequently *must* yield the same behavior, both in terms of messages and on-disk configuration

* the config-changed hook is run right after the install one (see[0]), although whether sysconfig correctly fires the config_changed() function when the config-changed hook is run is a separate issue

* the way I see it, being careful about when we do or we do not run the system.conf update function is very fragile. We should rather make update_systemd_system_file() able to be run at any point, safely, without triggering unnecessary 'reboot required'. Since systemd conf files are INI-like, they can easily be parsed with configparser. What I think you should do is have the charm:
  1. render the template, preferably in memory, or in a tempfile
  2. load the rendered template in a configparser object
  3. load system.conf in another configparser object
  4. compare the two configparser objects; if they are different, mv the tempfile into place and trigger the reboot-required notification

I have done a very quick check with two semantically identical systemd.conf files (only the order of the keys changed) and yes, something like this does work:

```
c_old = configparser.ConfigParser()
c_new = configparser.ConfigParser()
c_old.read('/etc/systemd/system.conf')
c_new.read('/some/temp/file')

c_old == c_new # returns True if the configs are semantically equal
```

[0] https://juju.is/docs/sdk/events#heading--lifecycle-event-triggers

review: Needs Fixing

« Back to merge proposal