Open Stack

On Wed, Jul 3, 2013 at 6:50 AM, Michael Still <mikal at stillhq.com> wrote:

> On Wed, Jul 3, 2013 at 3:50 AM, Boris Pavlovic <boris at pavlovic.me> wrote:
>
> > Question:
> >   Why we should put in oslo slqlalchemy-migrate monkey patches, when we
> are
> > planing to switch to alembic?
> >
> > Answer:
> >    If we don’t put in oslo sqlalchemy-migrate monkey patches. We won't be
> > able to work on 7 point at all until 8 and 10 points will be implemented
> in
> > every project. Also work around 8 point is not finished, so we are not
> able
> > to implement 10 points in any of project. So this blocks almost all work
> in
> > all projects. I think that these 100-200 lines of code are not so big
> price
> > for saving few cycles of time.
>
> We've talked in the past (Folsom summit?) about alembic, but I'm not
> aware of anyone who is actually working on it. Is someone working on
> moving us to alembic? If not, it seems unfair to block database work
> on something no one is actually working on.
>

That's not quite what happened. Unfortunately the conversation happened in
gerrit, IRC, and email, so it's a little hard to piece together from the
outside.

I had several concerns with the nature of this change, not the least of
which is it is monkey-patching a third-party library to add a feature
instead of just modifying that library upstream.

The patch I objected to (https://review.openstack.org/#/c/31016) modifies
the sqlite driver inside sqlalchemy-migrate to support some migration
patterns that it does not support natively. There's no blueprint linked
from the commit message on the patch I was reviewing, so I didn't have the
full background. The description of the patch, and the discussion in
gerrit, initially led me to believe this was for unit tests for the
migrations themselves. I pointed out that it didn't make any sense to test
the migrations on a database no one would use in production, especially if
we had to monkey patch the driver to make the migrations work in the first
place.

Boris clarified that the tests were the general nova tests, at which point
I asked why nova was relying on the migrations to set up a database for its
tests instead of just using the models. Sean cleared up the history on that
point, and although I'm still not happy with the idea of putting code in
oslo with the pre-declared plan to remove it (rather than consider it for
graduation), I agreed that the pragmatic thing to do for now is to live
with the monkey patched version of sqlalchemy-migrate.

At this point, I have removed my -2 to the patch, but I haven't had a
chance to fully review the code. I voted 0 to unblock it in case other
reviewers had time to look at it before I was able to come back. That
hasn't happened, but the patch is no longer blocked.

Somewhere during that conversation, I suggested looking at alembic as an
alternative, but alembic clearly states in its documentation that
migrations on sqlite are not supported because of the database's limited
support for alter statements, but that if someone wants to contribute those
features patches would be welcome. If we do need this feature to support
good unit tests of SQLalchemy-based projects, we should eventually move it
out of oslo and into alembic, then move our migration scripts to use
alembic. It would make the most sense to do that on a release boundary,
when we normally collapse the migration scripts anyway. Even better would
be if we could make the models and migration scripts produce databases that
are compatible enough for testing the main project, and then run tests for
the migrations themselves against real databases as a separate step. Based
on the plan Boris has posted, it sounds like he is working toward both of
these goals.

Doug


>
> Michael
>
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20130703/b60e79e4/attachment.html>