Code review comment for lp:~jtv/maas/1.2-bug-1069734

Looks generally good… but I'm wondering about how you tested this, hence "Needs information".

[0]

560 + rmtree(
561 + os.path.join(settings.MEDIA_ROOT, upload_dir),
562 + ignore_errors=True)

I think this is a bit dangerous, because it uses a user setting. Imagine the case where a user is installing MAAS (with a version which has this migration) but changes MEDIA_ROOT to a custom location. This migration, without any good reason, will issue a rm -rf ${MEDIA_ROOT}/storage. I really wonder if we shouldn't leave that directory alone. The flip side would be that old installations will have a 'storage' directory with a few files in it but new installations (which is probably what we should focus on) will be fine (and the rf -rf ... won't be run).

[1]

Testing: migrations cannot be unit tested but I'd be more comfortable with this change knowing that the migration has been tested on a instance with real data in FileStorage. How did you test this?