Juju Charms Collection
ntp package

Code review comment for lp:~brad-marshall/charms/trusty/ntp/add-auto-peers

  1. Trusty Tahr (14.04)
  2. add-auto-peers
  3. Merge into trunk
Revision history for this message
Cory Johns (johnsca) wrote :

Brad,

This change looks mostly good, and the merge conflict has been resolved. However, I have two items that I think should be addressed before I can give my +1:

  * Missing default value for nagios_servicegroups option (seems like it was missed / removed in the conflict resolution)

  * A test case for the auto-peering should be added (maybe just a couple of lines could be added to 10-deploy-test.py?)

I'm also a little confused about the purpose of the "total_sources" option? Is it really useful to have an option whose only effect is to create a log message? Is there more behavior that will be coming to this option?

review: Needs Fixing

« Back to merge proposal