Code review comment for lp:~gandelman-a/ubuntu/oneiric/cobbler/lp850880-850866

Revision history for this message
Scott Moser (smoser) wrote :

Some comments:
 * I don't like the use of globals in update_available and get_isos. I dont have a simple solution for you, and i wouldnt nack it completely based on that, but globals (or lexical scoped variables) are just confusing. If you have to use them, please use upper case names to indicate that they're global.
 * use more quotes:
   Things like '[ -n $md5sum_local ]' should really be '[ -n "$md5sum_local" ]', as the variable md5sum_local could have either shell wildcards or spaces in it which change behavior unexpectedly.
 * there are some white space indentation rather than tab (search for '[ ][ ][ ]' and you'll see).
   Also, you can set vi to read the modelines (the last line of the file) with 'set modeline modelines=3'

Also, I'd like a general "just do the right thing" flag.
So that I could do something like:
  cobbler-ubuntu-import --do-what-i-mean natty-i386 natty-amd64 oneiric-amd64 oneiric-i386

And it would import or update if necessary, and exit success unless an operation failed. It would even make sense to me that that should be the default behavior.

The last comment is that it would be nice if 'cobbler import' had a '--replace' flag. We might want to do that and submit it upstream that did your rename dance inside. It would be able to do more appropriate locking and such to avoid two things attempting the call at the same time.

Thanks for your work, I really hope not to sound negative.

review: Needs Fixing

« Back to merge proposal