-
Notifications
You must be signed in to change notification settings - Fork 14
Add ability to Squash Changes when merging #370
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
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
@arthanson, looks like you have a CI failure due to some lingering merge conflict markers. |
Contributor
|
Some quick testing notes... Test 1
Test 2
Test 3
|
jeremystretch
requested changes
Dec 8, 2025
jeremystretch
approved these changes
Dec 17, 2025
Contributor
|
FWIW, this looks good. Just holding off on approval/merging until I have |
jnovinger
approved these changes
Dec 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #198
This squashes (collapses) all Changelogs (for a given object) in a branch to a single Change:
The old (iterative) and new (collapse) merge strategy are both supported (the default is the old iterative approach). There is a select-box on merge to enable the squash merge:
Because ObjectChanges can reference other objects (FK) it has to do dependency ordering: If an object references something that is created, the create has to come first, etc..
This will not fix 100% of the issues, but the goal is that the end user can fairly easily fix the branch (by either deleting a problem Object or modifying it's data) and be able to get the branch to merge.
The biggest thing this fixes is Where you have an Object that causes a Constraint error. (Site with a slug that conflicts with one in main). Currently using the replay of changelogs you cannot fix this as you will always try to replay the ObjectcChange with the conflicting state. With collapse migrations, the intermediate state of that caused the conflict is skipped.
The major thing this can't fix is the swap case: You have a Module Bay with two modules at position 1 and 2 and you swap these (move 1 to 3, 2 to 1, 3 to 2) as with collapse you miss the move to position 3 and no matter which one you try to merge you will run into a constraint error. But, you can easily fix this by temporarily modifying the data before merge then merging.
Note: I looked at doing partial collapse (i.e. only collapsing some of the changes) but quickly ran into issues because of the constraint issues mentioned above and that Objects can reference other objects which means you have to do a dependency change and at that point it is just cleaner to collapse all the migrations as you have to do all the work anyways and it gets messy figuring out what you can partially collapse.
Step1: For collapse you want to take the initial objects prechange_data, and then you want to collapse all the objects postchange_data down into one so later field modifications overwrite older ones.
Note: because you are collapsing you sort of loose the time sequence (do you take it from the first change or the last - which actually depends). So barring dependencies we generally do DELETEs, UPDATEs then CREATEs (within those in time order)
Step 2: For dependency ordering it matters what type of operation you are doing. For a create you care about if postchange_data from another object refers to it, but for a delete you care if the prechange_data for another object refers to it. Note: we only care if these references are to other objects being modified in this branch. If it is a reference to something in main we don't need to track the dependency.
Step 3: For actually doing the dependency ordering a given Change can reference more then one FK so we use an algorithm that handles this and have to have cycle detection, which hopefully shouldn't happen (A references B, references C, references A) but need to guard against it.
Note: There is an edge case with nullable FK fields and having a created item refer to each other (Circuit / CircuitTermiantions apparently do this) If we detect this we split the create back into a create (with the nulled fields) and an update setting those fields.
Note: All of these new ObjectChange are in-memory, we don't actually modify the existing ObjectChange in the database. Because of this we can't use the existing apply / revert code as those work on actual ObjectChange records, so we have new functions similar to those.
_build_fk_dependency_graphhas very similar code for each type of Change, but it is just dissimilar enough I didn't see a good way of refactoring it to be more DRY without increasing the complexity. I also created some sub-functions which are only called once (like_build_fk_dependency_graph) but it encapsulates the given functionality and makes it far easier to understand IMHO.Right now there is a lot of logging that will appear in the Merge job log making it fairly big. This info is useful for debugging and it was decided to error on the side of too-much info rather than paring it down. We can do that later after it has been out and proven itself.
Note: From review changes - updated so squash revert / merge use the requesting user as the user for the ChangeLogs.