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

Move the migrations into the Bodhi package #1777

Merged
merged 1 commit into from Sep 11, 2017

Conversation

jeremycline
Copy link
Member

Alembic makes it easy to distribute the migrations as part of your
Python package. It also makes configuration easy since it uses
pkg_resources to find the migrations, rather than having the user
configure the path as part of packaging.

Signed-off-by: Jeremy Cline jeremy@jcline.org

@jeremycline
Copy link
Member Author

I just moved it to bodhi.server.migrations, but we could have bodhi.server.db with the migrations sub-package and the models. I don't have a strong opinion about which is nicer.

Copy link
Contributor

@bowlofeggs bowlofeggs left a comment

Choose a reason for hiding this comment

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

Please do mention the alembic.ini change in the release notes before merging.

@@ -3,6 +3,7 @@ source = bodhi
omit =
bodhi/scripts/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to this PR, but this path doesn't seem to exist.

@@ -2,7 +2,7 @@

[alembic]
# path to migration scripts
script_location = alembic
script_location = bodhi:server/migrations
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a backwards-incompatible change for the RPM, since alembic.ini is probably marked as a configuration file. This means that users who upgrade will not get this change and the migrations will fail. Two possible solutions I can think of quickly:

  1. Don't mark this as a config file. Does it have anything an end user should want to change in it? I'm not sure.
  2. Provide a symlink from the old location to the new location so things work smoothly, and drop the symlink in Rawhide. We already do this with another symlink on F26.

I'd probably opt for the second one.

^^^^^^^^^^^^^^^^^^^^^^^^

* The database migrations are now shipped as part of the Python distribution
(`#1777 <https://github.com/fedora-infra/bodhi/pull/1777>`_).
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to mention the alembic.ini change here.

@jeremycline jeremycline force-pushed the migration-subpackage branch 2 times, most recently from 38d9308 to 988e055 Compare September 11, 2017 15:14
Alembic makes it easy to distribute the migrations as part of your
Python package. It also makes configuration easy since it uses
pkg_resources to find the migrations, rather than having the user
configure the path as part of packaging.

Signed-off-by: Jeremy Cline <jeremy@jcline.org>
@fedora-infra fedora-infra deleted a comment from centos-ci Sep 11, 2017
@fedora-infra fedora-infra deleted a comment from centos-ci Sep 11, 2017
@jeremycline jeremycline merged commit 28dc459 into fedora-infra:develop Sep 11, 2017
@jeremycline jeremycline deleted the migration-subpackage branch September 11, 2017 19:02
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

2 participants