Code review comment for lp:~julian-edwards/maas/localboot-amt

Thanks for the review!

On Tuesday 14 Oct 2014 07:14:39 you wrote:
> Review: Approve
>
> Nicely done! I've tested this on my NUCs as well and it works as expected.

Ta.

> > +# boot_mode tells us whether we're pxe booting or local booting. For
> > local
> > +# booting, the argument to amttool must be empty (NOT 'hd', it doesn't
> > work!). +if [ "$boot_mode" = 'local' ]; then
> > + boot_mode=''
> > +fi
>
> The discrepancy between the passed 'boot_mode' and what the amt command uses
> makes reusing the same variable awkward I think. How about using
> 'boot_mode' as the name of the variable MAAS passes and something else, for
> instance 'amt_boot_mode', for the parameter AMT consumes? I think it would
> make the template more readable.

Sure. I was just trying to keep the template concise.

> > +
> > + # boot_mode is something that tells the template whether this is
> > + # a PXE boot or a local HD boot.
>
> I'd explain that this is something that the power templates can use but that
> it is mostly useful for the power types that don't support having a PXE
> config that says "Boot from local disk." AFAIK, none of the other power
> types need this: they can always PXE boot and they simply use a special PXE
> config when MAAS wants the nodes to boot from local disk.

Actually most power types do support this! IPMI is already using it, but it
uses a fixed config file. It would be nice to drive it from here instead at
some point.