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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
