Code review comment for lp:~frankban/lpsetup/bug-1034602-handle-target-dir

Revision history for this message
Francesco Banconi (frankban) wrote :

Thanks for your review Brad. I fixed the branch following your suggestions with one exception:
I've seen that removing the chmod clean up generates this error:

======================================================================
ERROR: test_is_writable (lpsetup.tests.test_handlers.HandleTargetDirTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/shutil.py", line 245, in rmtree
    rmtree(fullname, ignore_errors, onerror)
  File "/usr/lib/python2.7/shutil.py", line 254, in rmtree
    onerror(os.rmdir, path, sys.exc_info())
  File "/usr/lib/python2.7/shutil.py", line 252, in rmtree
    os.rmdir(path)
OSError: [Errno 13] Permission denied: '/tmp/tmp6TfXhT/utilities'

So, the write permission missing does not prevent you from deleting the directory iself, but its contents.
The chmod cleanup is called before the rmtree cleanup because the (documented) system used is LIFO.
Maybe a try/finally block could be more explicit?

review: Approve

« Back to merge proposal