search

Bogdanp / django_dramatiq

A Django app that integrates with Dramatiq.
https://dramatiq.io
Other
331 stars 77 forks source link

Fix #123 -- init broker configuration before loading other app models #126

Closed amureki closed 1 year ago

amureki commented 1 year ago

Bug from #123 was introduced in https://github.com/Bogdanp/django_dramatiq/pull/103 where django_dramatiq started benefitting from AppConfig.ready() functionality. There was a research done in https://github.com/Bogdanp/django_dramatiq/issues/100 explaining the underlying issue.

My PR is using the idea from @dnmellen to call DjangoDramatiqConfig.ready() during its models import, so earlier than any other custom models code would be imported. I was trying to come up with a different way, as this one feels tiny bit "hacky", however could not find anything better.

amureki commented 1 year ago

@Bogdanp hey Bogdan! Would be happy to get your input here. Also, would be great to trigger CI jobs to check the PR.

Best, Rust

Bogdanp commented 1 year ago

Thanks! This looks reasonable. I’ll take a deeper look this Sunday. I don’t see an option to trigger CI right now. Maybe the workflow isn’t set up to run on PRs (if not, that would also be a good change to make).

amureki commented 1 year ago

Something is odd with locks in test database. Seems flaky to me, but I am not yet familiar enough with the codebase to quickly find out the reason.

django.db.utils.OperationalError: database table is locked: django_dramatiq_task
Bogdanp commented 1 year ago

Something is odd with locks in test database. Seems flaky to me, but I am not yet familiar enough with the codebase to quickly find out the reason.

django.db.utils.OperationalError: database table is locked: django_dramatiq_task

Yes, I think those tests have always been a little flaky so I wouldn't worry too much about them at the moment.

Bogdanp commented 1 year ago

Looks good to me. Thanks again!