-
Notifications
You must be signed in to change notification settings - Fork 212
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
Add django-migration-git-conflict to prevent divergent migration merges #4075
Conversation
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.
LGTM, except that Pipenv has, once again, updated a ton of unrelated dependencies. Please see if it is possible to reduce the extent of the updates, but if not, I don't mind.
Oh gosh 🤦 I thought I checked to make sure, but I think I forgot to re-do it when I had to re-install the dependency locally due to a misconfiguration thing on my end. I'll go through it again, thank you for catching it. |
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.
TIL, very cool plugin. LGTM!
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.
Applying a change requested review just so we don't accidentally merge this PR which updates dependencies where we might affect production!
I was not requesting changes because it seems like the package upgrades are OK now that #4032 was merged. As far as I can tell, there are now no occurrences of asyncio tasks being created. |
5ca5e39
to
e2818fa
Compare
This PR has migrations. Please rebase it before merging to ensure that conflicting migrations are not introduced. |
Fixes
Fixes #687 by @sarayourfriend
Description
Adds
django-migration-git-conflict
recommended in this comment by @tylerlaprade (thanks!).Check out the plugin's repository for information on how it works: https://github.com/crux-lab/django-migrations-git-conflicts?tab=readme-ov-file#how-it-works
To summarise: the latest migration files for each app will get updated when a new migration is added. When that file is merged, any other branches with divergent latest migrations will conflict, and need a rebase to fix.
Existing CI checks should all continue to work, and we can leave in the PR comment requesting a rebase for migrations anyway, as an additional helpful reminder. Ideally PRs with migrations are always rebased even without a conflict so that the migrated code is tested against main, in case there are issues with new code unaware of the migration changes (i.e., a semantic conflict). But we are always vulnerable to those, even outside the context of a migration, so I don't think we should consider them a special case with respect to the linked issue.
Testing Instructions
Review the new files. Run
just api/dj makemigrations --check
locally and see that it works. Make a change to force a migration (change some help text on a model attribute, for example) and run makemigrations again, confirm that the latest migration is updated. All of that is really just showing how the plugin works, we don't need to test the plugin.Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin