Code review comment for lp:~termie/nova/db_migration

Heyo.

Mostly good (and necessary!) stuff, Andy. Nice job.

Here's a couple things to address, though (outside of the merge stuff you're currently looking at):

1) Please add sqlalchemy-migrations to tools/pip-requires otherwise tests will bomb in a virtualenv

2) Copy/paste docstring fail here :)

22 + def sync(self, version=None):
23 + """adds role to user
24 + if project is specified, adds project specific role
25 + arguments: user, role [project]"""
26 + return migration.db_sync(version)

3) Nova != Django :)

135 === added file 'nova/db/sqlalchemy/migrate_repo/__init__.py'
136 === added file 'nova/db/sqlalchemy/migrate_repo/manage.py'
137 --- nova/db/sqlalchemy/migrate_repo/manage.py 1970-01-01 00:00:00 +0000
138 +++ nova/db/sqlalchemy/migrate_repo/manage.py 2011-01-15 02:00:45 +0000
139 @@ -0,0 +1,4 @@
140 +#!/usr/bin/env python
141 +from migrate.versioning.shell import main
142 +if __name__ == '__main__':
143 + main(debug='False', repository='.')

Not sure if the above is necessary considering the nova-manage db sync. Was this just to achieve parity with Django? I didn't see this script being used anywhere.

4) This code:

951 +def db_sync(version=None):
952 + db_version()
953 + repo_path = _find_migrate_repo()
954 + return versioning_api.upgrade(FLAGS.sql_connection, repo_path, version)

Looks like there is something missing... db_version() returns the numeric version of the database, if it is versioned (returning 0 if not versioned). May I suggest changing the above code to the following:

def db_sync(version=1):
  cur_version = db_version()
  repo_path = _find_migrate_repo()
  if version > cur_version:
    return versioning_api.upgrade(FLAGS.sql_connection, repo_path, version)
  return True

Cheers!

jay