launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

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.