Code review comment for lp:~blake-rouse/maas/fix-bug-1300285

I think I understand what this does now, but the commit message is a bit cryptic. It summarises the code changes, but it doesn't give me much of a high-level idea of what the change means. It's that intention that's essential for review and maintenance. Could you perhaps say something like "make the PXE bootloader the default bootloader in the DHCP config, for when the node does not specify its architecture"? The extra sentence you provide in the description but not in the commit message is actually much more suitable for the commit message.

Also, there is no need to duplicate anything between the commit message and the description. The description is for the merge proposal only, and should help facilitate review. The commit message goes into revision history, with automatically added tags for author, reviewer, and bug number. Which also means: no need to prefix the bug yourself!

Typos in config.py: "sepcial" and, in that same comment, missing punctuation. Could you be a bit more descriptive? Someone reading this code and encountering "the special case PXEBootLoader" may still have trouble finding what that refers to. Are there several PXEBootLoaders, and is one of them a special case? Or do you mean that the PXEBootLoader is a special case?

In compose_conditional_bootloader, the loop structure and the code after the loop are now connected through the pxe_method variable, and through the "continue" statement in the loop. Those are often things worth avoiding, because it will take future maintainers time to figure out and preserve the intended behaviour. Not much, but it adds up — and what's more, they may not be aware of the need.

So might I suggest removing the "continue" and moving the variable closer to its point of use:

    for name, method in BootMethodRegistry:
        # PXEBootMethod is special. No matter what the arch_octet is,
        # it's always the last in the conditional, with an else. So
        # no matter the architecture it will always try to use pxelinux
        # to boot that node.
        if name != "pxe":
            output += CONDITIONAL_BOOTLOADER.format(
                behaviour=next(behaviour), arch_octet=method.arch_octet,
                bootloader=method.bootloader_path).strip() + ' '

    # Now put the PXE bootloader at the end.
    pxe_method = BootMethodRegistry.get_item('pxe')
    if pxe_method is not None:
        output += PXE_BOOTLOADER.format(
            bootloader=pxe_method.bootloader_path).strip()

(Also note that the "is its" in the comment could do with a comma.)

Alternatively, is it actually a problem to mention the PXE bootloader twice? If not, maybe repetition isn't so bad. That would give us something like:

    for name, method in BootMethodRegistry:
        output += CONDITIONAL_BOOTLOADER.format(
            behaviour=next(behaviour), arch_octet=method.arch_octet,
            bootloader=method.bootloader_path).strip() + ' '

    # Fall back on the PXE bootloader if architecture is unknown.
    pxe_method = BootMethodRegistry.get_item('pxe')
    if pxe_method is not None:
        output += PXE_BOOTLOADER.format(
            bootloader=pxe_method.bootloader_path).strip()

I'll trust your domain knowledge as to whether that's acceptable, but if it is, the code ends up being simpler and therefore safer!

Jeroen