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.
--
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/2048876
Title:
Allow server and pool sources to be overridden through a conf.d or
sources.d configuration
To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/chrony/+bug/2048876/+subscriptions
--
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
