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

Looks good. A few remarks:

[0]

99 + if [ -f $dest/pxelinux.0 ] && cmp pxelinux.0 $dest/pxelinux.0

I think you want to add "--quiet" to "cmp pxelinux.0 $dest/pxelinux.0" since you only care about the return code.

[1]

124 + rm -rf -- $dest/$name.old $dest/$name.new

How about using '-f' when moving files instead of cleaning up the files?

[2]

125 + # Put new downloads next to the old. If any file copying is needed

You've written the doc for this method without mentioning 'downloads'. I realize in practice this is used to move downloaded files but still, for consistency, I think you should s/downloads/directory/ here.

[3]

192 + cd -- $DOWNLOAD_DIR

Please consider using pushd/popd to restore go back to the original directory just before the script exits.

[4]

200 + # Replace the "base" with sub-architecture once we start supporting
201 + # those.
202 + arch_dir="$TFTPROOT/maas/$arch/base"
203 +
204 + mkdir -p -- $arch_dir
[…]
209 + rel_dir="$arch_dir/$release"
210 + mkdir -p -- $rel_dir
211 + update_install_files $rel_dir $arch $release

There is an additional white space on lines 202, 209 and 210.

[5]

259 + with open(os.path.join(path, name)) as infile:

Please consider using "open(os.path.join(path, name), 'rb')" for maximum compatibility.

[6]

196 + umask a+r

That looks like an important feature of the script. Don't you think it's worth a test?

[7]

66 +TFTPROOT=${TFTPROOT:-/var/lib/tftpboot/}
and
202 + arch_dir="$TFTPROOT/maas/$arch/base"

arch_dir will be /var/lib/tftpboot//maas/$arch/base. I know it's not a problem but maybe you'll want to remove that '//'.

[8]

Same "problem" with the download path: it downloads http://archive.ubuntu.com/ubuntu/dists//lucid/main/installer-amd64/current/images/netboot/ubuntu-installer/amd64//linux

[9]

54 +# Ubuntu releases that are to be downloaded.
55 +SUPPORTED_RELEASES=$(distro-info --supported)

Just a question here, is MAAS really supporting pre-precise releases?

[10]

70 +DOWNLOAD=${DOWNLOAD:-wget --no-verbose}

You've used --no-verbose and not --quiet here. This means that the output of this script looks like this: http://paste.ubuntu.com/1042263/. I think it's fine but I just wanted to make sure that this was your intention. The only weird thing is that it says "Updating files for oneiric-i386." after the log of the download is displayed. No biggie though.

[11]

I've noticed you've left the existing 'maas-import-isos' untouched. I understand we probably want to keep it around until the Cobbler replacement is done. But don't you think we should file bugs to make sure we don't forget to cleanup things when the replacement will be completed? I'd like your opinion on this because I've got the exact same dilemma about preseed templates/snippets.