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

Revert "Delete create_db.py (#1000)" #1152

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

abandoned-prototype
Copy link
Collaborator

Fixes issue

#1000 unfortunately broke alembic's functionality to create migrations that include new tables (Using flask db migrate). The issue is that before alembic has a change to analyze the difference between existing database and schema from database.py the db.create_all() command automatically creates the tables making it so that they are not registered for the migration.

I could not come up with a way to reasonably work around this issue, so I think the best course of action is to revert the removal of the create_db.py file.

Description of Changes

This reverts that breaking commit. It is not a clean revert of #1000, since that PR also included unrelated changes to tests and constants. Those changes were not reverted.

Notes for Deployment

I have verified the changes in my local environment and the tests are passing, but I have not tested that the changes work as expected in a completely fresh environment (which would use the create_db file)

Tests and Linting

  • This branch is up-to-date with the develop branch.
  • (N/A) Ran make create_db_diagram command.
  • pytest passes on my local development environment.
  • pre-commit passes on my local development environment.

This change unfortunately broke alembic's functionality to create migrations that include new databases.

This reverts commit 495a9a0.
It is not a clean revert of PR #1000/commit 495a9a0, since that PR also included unrelated changes to tests and constants. Those changes were not reverted.
@@ -52,7 +52,7 @@ repos:
description: Only executable files should have shebangs (e.g. '#!/usr/bin/env python')
entry: "#!/"
pass_filenames: true
exclude: bin|cli|manage.py|app.py|setup.py|docs|test_data.py
exclude: bin|cli|manage.py|app.py|setup.py|docs|create_db.py|test_data.py
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can leave create_db off of this list, yeah?

@@ -19,6 +14,12 @@
logger = logging.getLogger("alembic.env")


config.set_main_option(
"sqlalchemy.url", current_app.config.get("SQLALCHEMY_DATABASE_URI")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you change this to: "sqlalchemy.url", current_app.config.get(KEY_DATABASE_URI)?

from OpenOversight.app.models.database import db


app = create_app("development")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can be KEY_ENV_DEV.

@@ -0,0 +1,9 @@
#!/usr/bin/python
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need the shebang, right?

Copy link
Collaborator

@michplunkett michplunkett left a comment

Choose a reason for hiding this comment

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

If you could make those small shebang and constant related changes, it looks good to go!

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.

2 participants