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

fix(migration): fix migration when another model with a different id exists #160

Conversation

croissong
Copy link
Contributor

fixes #159

@croissong croissong force-pushed the fix/fix-migration-with-another-model branch 2 times, most recently from 6d2f80f to 91276ea Compare June 11, 2024 16:04
@vincent-herlemont
Copy link
Owner

The CI failed due to a token issue, audit in progress here: #161

@vincent-herlemont
Copy link
Owner

The CI problem is not related to the code, so it's fine by me.

@croissong You can mark the issue as ready for review if you want me to merge it.

@croissong
Copy link
Contributor Author

croissong commented Jun 11, 2024

@vincent-herlemont thanks!
I added two slightly unrelated commits just now:

  • tried to improve the "Impossible to migrate the table ..." error message, because it confused me a bit when I encountered it, and I think it should be made clear that the issue is caused by multiple old tables with data.
    I hope the error is at least a little bit better now 😅

  • additional tests for migrating with multiple model versions, to make sure the migrate logic still works after the change.
    But I just noticed that there's already test_migrate_v3 which covers multiple model versions, so let me know if I should change/remove my tests

@croissong croissong marked this pull request as ready for review June 11, 2024 17:13
@croissong croissong force-pushed the fix/fix-migration-with-another-model branch from c183d77 to 11d094b Compare June 11, 2024 17:59
@vincent-herlemont vincent-herlemont merged commit 284ff1d into vincent-herlemont:main Jun 12, 2024
8 of 9 checks passed
@vincent-herlemont
Copy link
Owner

vincent-herlemont commented Jun 12, 2024

@croissong Thank you for improving the error message and adding the tests!

But I just noticed that there's already test_migrate_v3 which covers multiple model versions, so let me know if I should change/remove my tests

Yes, sorry for the naming of some of my tests like test_migrate_v3, which are not very descriptive. I should pay more attention to that.

This will be taken into account for Release 0.7.0. However, I am currently doing a major refactor, so it should be ready in one or two weeks.

@vincent-herlemont
Copy link
Owner

🎉 This PR is included in version 0.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migration fails when another model with a different id exists
2 participants