Code review comment for lp:~jtv/maas/import-to-tftp

Revision history for this message
Robie Basak (racb) wrote :

This is really readable and shouldn't be too difficult to add ARM support to cleanly - thanks!

Can we please use "generic" instead of "base"? "base" sounds like it is used as the foundation for something else which I think is a bit misleading. The armhf subarchitectures correspond to kernel flavours and "generic" is the standard i386/amd64 flavour that we are moving to so I think this makes sense.

ARCHIVE=${ARCHIVE:-http://archive.ubuntu.com/ubuntu/dists/} should skip the dists at the end - the standard way of specifying an archive is one level higher than this, without the dists. Then compose_installer_download_url would insert "dists" into the URL instead.

ARM support:

I'm happy to work on these as a separate merge request. Here are the changes I think we'll need:

The list of supported arches will need to grow from amd64 and i386 to armhf/highbank, armhf/armadaxp etc. At this point, it might be cleanest to specify ARCHES as "amd64/generic i386/generic armhf/highbank" etc.

The archive needs to be selected based on the architecture in use, in order to switch from archive.u.c/ubuntu to ports.u.c/ubuntu-ports. Can this be made into a function, rather than directly a variable?

compose_installer_download_url will need to use a lookup table as the exact URL differs based on arch and release. The needed URLs don't necessary end in pxelinux.0, linux and initrd.gz though, so I think this needs to be a bit more specialised and return the actual URL of the kernel image, initrd image and pxelinux.0 instead. For example: the armadaxp kernel image is called uImage because it is in a different format.

« Back to merge proposal