Open Stack

On Mon, 2013-11-18 at 18:46 +0100, Nikola Đipanov wrote:
> Dear OpenStack devs,
> 
> A recent review [1] dragged into spotlight how damaging improper use of
> external code inside migrations can be.
> 
> Basically in my mind the incident raises 2 issues that I think we should
> look into:
> 
> 1) How can we make reviewing changes with db migrations more robust
> 
> Since we use sqlalchemy-migrate to version our database, the package's
> documentation [2] states how care needs to be taken when importing code
> inside a db migration script. It seems like this can be taken care of
> with a hacking rule that will fail the patch if anything outside a
> subset of modules is imported and used. I might be missing an angle
> where such an approach could cause issues, so feel free to comment in
> replies. IIUC - this is something we might want to enforce even when/if
> we move to using Alembic for migrations.
> 
> 2) What are acceptable changes
> 
> The patch also raised the question of what is acceptable level of
> changes. The only guidelines I could find are [3] and they seem fuzzy
> enough that we might want to be more specific, or introduce stricter
> testing guidelines.
> 
> All comments are more than welcome,
> 
> Thanks,
> 
> Nikola
> 
> [1] https://review.openstack.org/#/c/39929/
> [2]
> https://sqlalchemy-migrate.readthedocs.org/en/v0.7.2/versioning.html#edit-the-change-script
> [3] https://wiki.openstack.org/wiki/DbMigrationChangeGuidelines

Thanks for the link Nikola.

It's pretty much as I guessed in the review. The intent is:

  https://sqlalchemy-migrate.readthedocs.org/en/v0.7.2/versioning.html#writing-scripts-with-consistent-behavior

  "You don’t want your change scripts’ behavior changing"

That totally makes sense. You don't want one version of migration 173 to
do something different from another version of migration 173.

One approach to achieving consistent behaviour is to only import library
APIs whose behaviour never changes and never change the migration's
code.

Another approach is to carefully review any changes which have the
potential to cause migration behaviour changes.

Another approach is to add tests for the behaviour so that if you can't
accidentally change the behaviour.

The point is we need to be careful to avoid changes in behaviour. By all
means, let's have guidelines which capture our experience around how
best to do that ... but let's not pick one of those approaches and
enforce it in such a way that leaves no room to - if needed - use a
different approach to achieve the same thing.

This isn't a question of style, where multiple approaches are valid and
we just need to pick one to avoid nitpicking and arbitrary
inconsistency.

Mark.