Comment 12 for bug 2048876

Some review comments:

0. I'm ok if you prefer to handle this as debdiff, but the Server team traditionally uses git-ubuntu for MP reviews. If you're familiar with git you'll likely also find it more comfortable, however I understand if due to the time pressure you'd rather do it an already known way. But put git-ubuntu on your todo list to master, if it's not already.

1. As I'm studying more closely I see that the config for the NTP pool servers is part of the package delta that Christian added, so that is going to be less relevant to Debian, and also I see that Debian provides the d/sources.d/ directory that you're using already, so while I did initially think like the others that this should be done in Debian, I'm seeing now it's more relevant to us only. It can't hurt to discuss with Debian but it's highly doubtful they'll take our delta. Also, since they already provide the d/sources.d/ directory I'm less worried about divergence with them.

2. In the postinst, if you reverse the order of the if clauses you can drop the additional filecheck, i.e.:

+ if [ ! -f /etc/chrony/sources.d/default-ntp-pools.sources ];
+ then
+ db_input low chrony/configure_default_pools_in_sourcesd || true
+ db_go
+ db_get chrony/configure_default_pools_in_sourcesd
+ if [ "${RET}" = "true" ];
+ then
+ cp --preserve /usr/share/chrony/default-ntp-pools.sources /etc/chrony/sources.d/default-ntp-pools.sources
+ fi
+ elif ! cmp -s /usr/share/chrony/default-ntp-pools.sources /etc/chrony/sources.d/default-ntp-pools.sources;
+ then
+ db_input low chrony/preserve_user_configured_pools_in_sourcesd || true
+ db_go
+ db_get chrony/preserve_user_configured_pools_in_sourcesd
+ if [ "${RET}" = "false" ];
+ then
+ cp --preserve /usr/share/chrony/default-ntp-pools.sources /etc/chrony/sources.d/default-ntp-pools.sources
+ fi
+ fi

I also think that presents your change a bit clearer since on initial install the file doesn't exist in sources.d and will need copied in place, so that first stanza is what happens first.

3. "The maintainer's version and current version of /etc/chrony/default-ntp-pools.sources are different."

I'm not certain, but this verbage seems a bit off. Can you doublecheck how other packages doing this same operation word the statement? (For example, as this is addressed to an end user, I'm not sure they would understand "current version" means the version currently installed on their system, and might not know what we mean by 'maintainer'.) If other packages are using this same wording style it's fine, but if there's a more conventional phrasing I'd go with something closer to that.

4. You might check whether the postrm should also do `rm -rf /etc/chrony/sources.d/` when purging.

5. Typically something like this should also be accompanied with some documentation regarding the change, such as in README.*, sources.d/README, and/or NEWS.

6. Does chrony-helper require any alterations?

7. There are references to `man chrony.conf(5)` in the service file. Probably worth checking the man page to make sure this change doesn't make any of its text outdated, and if it does include a patch to adjust it. May potentially be something to include in the Test Case.

8. Why is --debconf-ok added for these two lines?

+ ucf --debconf-ok --three-way /usr/share/chrony/chrony.conf /etc/chrony/chrony.conf
+ ucf --debconf-ok --three-way /usr/share/chrony/chrony.keys /etc/chrony/chrony.keys

9. The changelog entry could probably benefit from some wordsmithing. But I'll leave that to the next iteration.