Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wip alembic: fix in transaction recipe #84

Merged
merged 1 commit into from
May 24, 2017

Conversation

dinosk
Copy link
Member

@dinosk dinosk commented May 23, 2017

  • Fixes creation of sequence in transaction table,
    to make it able to drop

  • Adds a utility function to drop the alembic_version table
    Closes #83

Signed-off-by: Dinos Kousidis konstantinos.kousidis@cern.ch

@nharraud nharraud self-requested a review May 23, 2017 13:26
@dinosk dinosk changed the title alembic: fix in transaction recipe wip alembic: fix in transaction recipe May 23, 2017
@dinosk dinosk force-pushed the recipe-fix branch 11 times, most recently from 3512820 to 5778c68 Compare May 23, 2017 17:31
@@ -78,3 +81,11 @@ def create_alembic_version_table():
alembic.migration_context._ensure_version_table()
for head in alembic.script_directory.revision_map._real_heads:
alembic.migration_context.stamp(alembic.script_directory, head)


def drop_alembic_version_table():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be useful if we also had a combined function drop_all() that does db.drop_all() and drop_alembic_version_table() so that we can use a single function from tests?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do it yes but it would mean that it wouldn't be advised anymore to use sqlalchemy create_all and drop_all as they are not tested. Normally there is only one "alembic recipe" test per module and only that one need to do the extra step of dropping the alembic_version table.
Also the CLI commands invenio db create and invenio db drop are already creating and dropping the alembic_version table in addition to calling create_all and drop_all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it :-) No need to add the function then.

@dinosk dinosk force-pushed the recipe-fix branch 6 times, most recently from 4b978d2 to 56e5986 Compare May 24, 2017 12:45
- Fixes creation of sequence in transaction table,
  so that it can be dropped.

- Adds a utility function to drop the alembic_version table.

Signed-off-by: Dinos Kousidis <konstantinos.kousidis@cern.ch>
@nharraud nharraud merged commit 221834d into inveniosoftware:master May 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants