launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Review: Needs Information

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? 
-- 
https://code.launchpad.net/~jtv/maas/1.2-bug-1069734/+merge/131334
Your team MAAS Maintainers is subscribed to branch lp:maas/1.2.