-
Notifications
You must be signed in to change notification settings - Fork 2
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
middleware: implement maintenance mode middleware (bug 1927149) #145
base: zeid/bug-1904616-update-uwsgi-config
Are you sure you want to change the base?
middleware: implement maintenance mode middleware (bug 1927149) #145
Conversation
unapplied_migrations = ( | ||
subprocess.run(["lando", "migrate", "--check", "--noinput"]).returncode != 0 | ||
) | ||
|
||
if force or unapplied_migrations: | ||
action_mapping[action]() |
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.
I wonder if the migrations should be a separate command, that toggles the maintenance mode as needed. This way we could turn on maintenance mode for any other reason where we might not want to add the weight of migrations.
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.
Alternatively, we could provide a flag such as --skip-migration
to achieve the same thing.
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.
That was kind of the intention of the force
parameter, though I do see your point. Maybe maintenance
should modify the flags regardless, and instead we can create pre-deploy
and post-deploy
commands which can do the migration check instead.
@@ -10,7 +10,7 @@ | |||
) | |||
|
|||
|
|||
def test_app_wide_headers_set(client): | |||
def test_app_wide_headers_set(db, client): |
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.
Where does db
come from? Is it a fixture? I can't find its definition (not that it impacts the tests).
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.
Good point. This is actually a fixture provided by pytest-django, though after going over the documentation again, it seems it is intended to be used in other fixtures, and not as a shortcut/alternative to marking the test with django_db
.
Though it works as expected, I think it's better to explicitly mark those tests as needing the db. I'll file a bug to fix other occurrences in the future, but will change these in this PR.
db
fixture to some testsget
method instead of returning full object