The code and tests look fine. However the log notice comes across as confusing for a reader who doesn't already know how things work. It currently says...
89 + "Unable to parse the DHCP leases file. You might want to install " 90 + "the 'maas-dhcp' package. Note that this is perfectly fine " 91 + "if this cluster is not managing its DHCP server.") I would switch those last two sentences around, so that the message conveys information in this order: 1. There's no leases file. 2. Which is fine, unless you want MAAS to manage DHCP. 3. If you want MAAS to manage DHCP, the problem is that maas-dhcp hasn't been installed. Several things need attention in the details of the text, in my view: * It doesn't help that the "this is perfectly fine" is ambiguous -- it could refer to the inability to parse the leases file, or to installation of maas-dhcp. * Is "Unable to parse the DHCP leases file" really the best information we can give? There's a big difference between "the leases file does not exist" and "the parser had a problem with the contents of the leases file." I think this message is only meant for the former case. * Hinting that people may be able to solve their problems by installing maas-dhcp is an open invitation for people to set up accidental rogue DHCP servers. * "Note that" doesn't carry a lot of meaning. Better leave it out. Please fix up before landing! Jeroen -- https://code.launchpad.net/~rvb/maas/bug-1061409/+merge/127979 Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/bug-1061409 into lp:maas. _______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
