MAAS

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

import-to-tftp
Merge into trunk

Revision history for this message

Jeroen T. Vermeulen (jtv) wrote on 2012-06-15:

Thanks for the unexpected additional review. I've made the changes you asked for:

> 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.

Just goes to show that no matter how many people you sanity-check with, there's always something you don't know about. I updated the code, and the PXE requirements document as well.

> 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.

Done. Gratifying to see how few changes that took.

You also mentioned some changes you might want to undertake yourself:

> 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.

Yes, something like that. Right now we can't quite do that because these settings are shared with the old, Cobbler-based scripts.

> 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?

I suppose. But it's a worry for later... Other solutions may become apparent in the meantime.

> 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.

A bit disheartening, but I guess we'll just have to deal with it when the time comes.

Jeroen

Thanks for the unexpected additional review.  I've made the changes you asked for:

Just goes to show that no matter how many people you sanity-check with, there's always something you don't know about.  I updated the code, and the PXE requirements document as well.

Done.  Gratifying to see how few changes that took.

You also mentioned some changes you might want to undertake yourself:

Yes, something like that.  Right now we can't quite do that because these settings are shared with the old, Cobbler-based scripts.

I suppose.  But it's a worry for later...  Other solutions may become apparent in the meantime.

A bit disheartening, but I guess we'll just have to deal with it when the time comes.

Jeroen

« Back to merge proposal