-
Notifications
You must be signed in to change notification settings - Fork 121
[Core Data] Remove old models. Part 2 (30-59) #16109
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
Conversation
Generated by 🚫 Danger |
|
|
|
I'll be AFK the rest of this week, if all looks good feel free to merge, otherwise I'll address any feedback next week 🙇 |
itsmeichigo
left a comment
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.
The test instruction seems to be outdated, so I just ran the code after installing the oldest build in TestFlight (22.5). The app didn't crash so 👍
|
Thanks for the review and testing!
The test instructions are outdated indeed, sorry for that 😅 , we simplified the behavior here: 6080964 so rather than having to set a constants to compare model versions, we just rely on the existence of the model itself. |

Apologies for the 32k LOC, it belongs mostly to deleting the .xcdatamodels and associated .xcmappings, the actual changes are the ~90 additions.
This PR continues the work of core data pruning started by #15987, in this one do a second pass by:
There is no updates to the migration paths or any logic has been changed, it's just a code removal & test updates.
Steps to reproduce/Testing information
As with the first part, this is not very straight-forward to test, but has been live for a month now so we should be certain there are no major issues.
Ultimately we'd want the merchant to not crash if they happen to be on a model version that has been deleted when a migration is performed. This crash is expected with an iterative migration if a step is missing, but won't happen with the direct migration. If the worst happens and they crash due being unable to perform iterative migration, the app already performs a direct migration as a fallback, which is what we leverage directly via the threshold check when we can.
I added instructions on the P2 at p91TBi-drE-p2 on how I initially tested it by deleting 120 models instead, since this was the oldest available in TestFlight it allows us to simulate what would happen when a migration is needed and the threshold is hit.
At the moment of writing this PR the latest available TF version is
22.5, which as per MIGRATIONS.md was usingmodel 121, so I'll use that as example. You can: