Open Stack

On Thu, Oct 31, 2013, Sean Dague <sean at dague.net> wrote:
> So there is a series of patches starting with -
> https://review.openstack.org/#/c/53417/ that go back and radically
> change existing migration files.
> 
> This is really a no-no, unless there is a critical bug fix that
> absolutely requires it. Changing past migrations should be
> considered with the same level of weight as an N-2 backport, only
> done when there is huge upside to the change.
> 
> I've -2ed the first 2 patches in the series, though that review
> applies to all of them (I figured a mailing list thread was probably
> more useful than -2ing everything in the series).
> 
> There needs to be really solid discussion about the trade offs here
> before contemplating something as dangerous as this.

The most important thing for DB migrations is that they remain
functionality identical.

Historically we have allowed many changes to DB migrations that kept
them functionally identical to how they were before.

Looking through the commit history, here's a sampling of changes:

- _ was no longer monkey patched, necessitating a new import added
- fix bugs causing testing problems
- change copyright headers
- remove unused code (creating logger, imports, etc)
- fix bugs causing the migrations to fail to function (on PostgreSQL,
  downgrade bugs, etc)
- style changes (removing use of locals(), whitespace, etc)
- make migrations faster
- add comments to clarify code
- improve compatibility with newer versions of SQLAlchemy

The reviews you're referencing seem to fall into what we have
historically allowed.

That said, I do agree there needs to be a higher burden of proof that
the change being made is functionally identical to before.

JE