And on to Raphaël's review!


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

Oh yes, absolutely.  That also means I no longer need to check for the file's existence.  I only did that to suppress the output — the documentation for --quiet suggested that the error message for a missing file would still be there, as you'd normally expect anyway, so I didn't think to use it.


> [1]
> 
> 124     +    rm -rf -- $dest/$name.old $dest/$name.new
> 
> How about using '-f' when moving files instead of cleaning up the files?

Won't work for (non-empty) directories.  There's even an option to make “mv” act as if the destination were a file, but I couldn't find a way to make that do what I wanted either.


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

Oops.  Older incarnation of the code, when this wasn't extracted yet.  Fixed.


> [3]
> 
> 192     +    cd -- $DOWNLOAD_DIR
> 
> Please consider using pushd/popd to restore go back to the original directory
> just before the script exits.

I did consider it at the time, but bear in mind that “cd” will only affect the process of the shell that was forked for the purpose of running the script.  The caller isn't affected.


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

Took me a while to figure it out, since I didn't find any extra space in my editor: I got tabs I never asked for!  All fixed now.


> [5]
> 
> 259     +    with open(os.path.join(path, name)) as infile:
> 
> Please consider using "open(os.path.join(path, name), 'rb')" for maximum
> compatibility.

Compatibility with what?  Windows?  Python 3?  Unusual encodings?  Given that this is for ASCII text files written from the same tests, what's the benefit of treating them as binary files?


> [6]
> 
> 196     +    umask a+r
> 
> That looks like an important feature of the script.  Don't you think it's
> worth a test?

Not particularly, no.  It just means that the script will write world-readable files even if your umask doesn't normally do that.  It doesn't affect its environment in any other way.


> [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 '//'.

I felt the trailing slash on $TFTPROOT was clearer, but if you have a problem with it, I'll remove it.


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

This was also a deliberate choice, but a subtly different one.  The URL returned by compose_installer_download_url refers to a directory-like resource, so considered as a standalone URL, it should have the trailing slash.  But I felt it would be unnecessarily confusing not to have a slash as the separator when adding the last path element.


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

Yes and no.  If you look in etc/maas/import_isos, you'll see SUPPORTED_RELEASES set to “precise.”  The plan is to add quantal there later.  But in principle, with an eye on the indeterminate future, the code does the sensible thing.


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

It's deliberate: the script first downloads the files, then sees if there are any changes, and only if there are will it announce that it's updating them (and update accordingly, of course).

The problem with wget --quiet is that it also suppresses error output.  Both wget and curl seem to be astoundingly awkward in this regard.


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

Remember: cleanup is a noun, clean up is a verb!

There is a separate card on the board for making maas-import-isos work without cobbler.  That job will consist of replacing maas-import-isos with this new script, once the other required parts are in place.

I don't like to worry too much about these small things in advance: if cleanup fits in naturally with the other work, we'll just do it and not worry about it.  If we discover that it doesn't, then that is a good time to file a bug or a card so we don't remember.  Until then, piling up detailed bugs or cards will just cloud our view of the work ahead.  And as always: no matter how clear-cut a remote problem seems now, expect our view of it to change by the time it becomes relevant.


Jeroen