Code review comment for lp:~blake-rouse/maas/osystem-preseed-cleanup

Thanks for doing this work. It's good to see those old hard-coded enums go at last. Unfortunately there's that pervasive problem of the registries living in a different process, possibly on a different machine. We should probably have been a lot clearer about that.

This branch answers a question I asked on one of your other merge proposals: does OperatingSystem really need to be a base class? If it's going to have a method for generating preseed data, and if it really can't be addressed with simple text templating, then it probably does.

An uncomfortable number of dimensional axes are coming together in compose_preseed: commissioning vs. deployment, operating systems with and without preseed code, conventional installer or fastpath (or whatever install options some other OS might have in their place). Gotta nip that sort of burgeoning complexity in the bud, or it will eventually stifle further development. Fight it with fire because by nature it will just keep growing.

So, assuming that we really do need OS-specific preseed-generating code, I would look for a way to move the Ubuntu-specific parts into the Ubuntu OperatingSystem. Or if most-but-not-all operating systems can generate their preseeds using templates, delegate most operating systems' implementations to a single template-based helper. (If templates can reasonably do the job for all operating systems we have in mind, of course, just use templating and stick with generic code.)

The hasattr check is best avoided, I think. If you're going to rely on dynamic dispatch for this, go all the way. You can accommodate some kind of sensible default in the dynamic method's definition, but an unconditional code path is going to be more manageable than special-casing in the code. Conditionals are our profession's equivalent of moving parts: they introduce complexity and risk.

There also seems to be a tacit rule in there that the commissioning OS must be, or work like, Ubuntu. That is worth being very explicit about. This isn't just a series of "if" statements to make things work, it's defining how a node can or cannot be installed. The details of of how these conditions interact are crucial, and yet I don't see any tests covering things like “we generate the customary Ubuntu preseed for a commissioning node, even if it's to be deployed with a different OS.”

By the way, I do appreciate how this branch is small and focused and not thrown in with a large bag of other changes!