-
Notifications
You must be signed in to change notification settings - Fork 78
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
base: develop
Are you sure you want to change the base?
Conversation
@@ -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 |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this 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!
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 thedb.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
develop
branch.make create_db_diagram
command.pytest
passes on my local development environment.pre-commit
passes on my local development environment.