This brings up an interesting problem: In https://review.openstack.org/#/c/70420/ I've added a migration that uses a drop column for an upgrade.
But savann-ci is apparently using a sqlite database to run. So it can't possibly pass. What do we do here? Shift savanna-ci tests to non sqlite? Trevor On Sat, 2014-02-01 at 18:17 +0200, Roman Podoliaka wrote: > Hi all, > > My two cents. > > > 2) Extend alembic so that op.drop_column() does the right thing > We could, but should we? > > The only reason alembic doesn't support these operations for SQLite > yet is that SQLite lacks proper support of ALTER statement. For > sqlalchemy-migrate we've been providing a work-around in the form of > recreating of a table and copying of all existing rows (which is a > hack, really). > > But to be able to recreate a table, we first must have its definition. > And we've been relying on SQLAlchemy schema reflection facilities for > that. Unfortunately, this approach has a few drawbacks: > > 1) SQLAlchemy versions prior to 0.8.4 don't support reflection of > unique constraints, which means the recreated table won't have them; > > 2) special care must be taken in 'edge' cases (e.g. when you want to > drop a BOOLEAN column, you must also drop the corresponding CHECK (col > in (0, 1)) constraint manually, or SQLite will raise an error when the > table is recreated without the column being dropped) > > 3) special care must be taken for 'custom' type columns (it's got > better with SQLAlchemy 0.8.x, but e.g. in 0.7.x we had to override > definitions of reflected BIGINT columns manually for each > column.drop() call) > > 4) schema reflection can't be performed when alembic migrations are > run in 'offline' mode (without connecting to a DB) > ... > (probably something else I've forgotten) > > So it's totally doable, but, IMO, there is no real benefit in > supporting running of schema migrations for SQLite. > > > ...attempts to drop schema generation based on models in favor of migrations > > As long as we have a test that checks that the DB schema obtained by > running of migration scripts is equal to the one obtained by calling > metadata.create_all(), it's perfectly OK to use model definitions to > generate the initial DB schema for running of unit-tests as well as > for new installations of OpenStack (and this is actually faster than > running of migration scripts). ... and if we have strong objections > against doing metadata.create_all(), we can always use migration > scripts for both new installations and upgrades for all DB backends, > except SQLite. > > Thanks, > Roman > > On Sat, Feb 1, 2014 at 12:09 PM, Eugene Nikanorov > <enikano...@mirantis.com> wrote: > > Boris, > > > > Sorry for the offtopic. > > Is switching to model-based schema generation is something decided? I see > > the opposite: attempts to drop schema generation based on models in favor of > > migrations. > > Can you point to some discussion threads? > > > > Thanks, > > Eugene. > > > > > > > > On Sat, Feb 1, 2014 at 2:19 AM, Boris Pavlovic <bpavlo...@mirantis.com> > > wrote: > >> > >> Jay, > >> > >> Yep we shouldn't use migrations for sqlite at all. > >> > >> The major issue that we have now is that we are not able to ensure that DB > >> schema created by migration & models are same (actually they are not same). > >> > >> So before dropping support of migrations for sqlite & switching to model > >> based created schema we should add tests that will check that model & > >> migrations are synced. > >> (we are working on this) > >> > >> > >> > >> Best regards, > >> Boris Pavlovic > >> > >> > >> On Fri, Jan 31, 2014 at 7:31 PM, Andrew Lazarev <alaza...@mirantis.com> > >> wrote: > >>> > >>> Trevor, > >>> > >>> Such check could be useful on alembic side too. Good opportunity for > >>> contribution. > >>> > >>> Andrew. > >>> > >>> > >>> On Fri, Jan 31, 2014 at 6:12 AM, Trevor McKay <tmc...@redhat.com> wrote: > >>>> > >>>> Okay, I can accept that migrations shouldn't be supported on sqlite. > >>>> > >>>> However, if that's the case then we need to fix up savanna-db-manage so > >>>> that it checks the db connection info and throws a polite error to the > >>>> user for attempted migrations on unsupported platforms. For example: > >>>> > >>>> "Database migrations are not supported for sqlite" > >>>> > >>>> Because, as a developer, when I see a sql error trace as the result of > >>>> an operation I assume it's broken :) > >>>> > >>>> Best, > >>>> > >>>> Trevor > >>>> > >>>> On Thu, 2014-01-30 at 15:04 -0500, Jay Pipes wrote: > >>>> > On Thu, 2014-01-30 at 14:51 -0500, Trevor McKay wrote: > >>>> > > I was playing with alembic migration and discovered that > >>>> > > op.drop_column() doesn't work with sqlite. This is because sqlite > >>>> > > doesn't support dropping a column (broken imho, but that's another > >>>> > > discussion). Sqlite throws a syntax error. > >>>> > > > >>>> > > To make this work with sqlite, you have to copy the table to a > >>>> > > temporary > >>>> > > excluding the column(s) you don't want and delete the old one, > >>>> > > followed > >>>> > > by a rename of the new table. > >>>> > > > >>>> > > The existing 002 migration uses op.drop_column(), so I'm assuming > >>>> > > it's > >>>> > > broken, too (I need to check what the migration test is doing). I > >>>> > > was > >>>> > > working on an 003. > >>>> > > > >>>> > > How do we want to handle this? Three good options I can think of: > >>>> > > > >>>> > > 1) don't support migrations for sqlite (I think "no", but maybe) > >>>> > > > >>>> > > 2) Extend alembic so that op.drop_column() does the right thing > >>>> > > (more > >>>> > > open-source contributions for us, yay :) ) > >>>> > > > >>>> > > 3) Add our own wrapper in savanna so that we have a drop_column() > >>>> > > method > >>>> > > that wraps copy/rename. > >>>> > > > >>>> > > Ideas, comments? > >>>> > > >>>> > Migrations should really not be run against SQLite at all -- only on > >>>> > the > >>>> > databases that would be used in production. I believe the general > >>>> > direction of the contributor community is to be consistent around > >>>> > testing of migrations and to not run migrations at all in unit tests > >>>> > (which use SQLite). > >>>> > > >>>> > Boris (cc'd) may have some more to say on this topic. > >>>> > > >>>> > Best, > >>>> > -jay > >>>> > > >>>> > > >>>> > _______________________________________________ > >>>> > OpenStack-dev mailing list > >>>> > OpenStack-dev@lists.openstack.org > >>>> > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > >>>> > >>>> > >>>> > >>>> _______________________________________________ > >>>> OpenStack-dev mailing list > >>>> OpenStack-dev@lists.openstack.org > >>>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > >>> > >>> > >> > >> > >> _______________________________________________ > >> OpenStack-dev mailing list > >> OpenStack-dev@lists.openstack.org > >> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > >> > > > > > > _______________________________________________ > > OpenStack-dev mailing list > > OpenStack-dev@lists.openstack.org > > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > > > > _______________________________________________ > OpenStack-dev mailing list > OpenStack-dev@lists.openstack.org > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev _______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev