Fix downgrade when normalized down revisions have overlap via depends_on #1376
Conversation
|
wow you got it? impressive job! thanks for digging into that code! |
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 64492e0 of this pull request into gerrit so we can run tests and reviews and stuff
|
New Gerrit review created for change 64492e0: https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5027 |
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 39f5ad0 of this pull request into gerrit so we can run tests and reviews and stuff
|
Patchset 39f5ad0 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5027 |
There was a problem hiding this comment.
Federico Caselli (CaselIT) wrote:
looks ok
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5027
| {self.bmerge.revision}, | ||
| ) | ||
|
|
||
| # Downgrading from bmerge to a1 should return back to heads={"b1"}. |
There was a problem hiding this comment.
Federico Caselli (CaselIT) wrote:
let's also try with downgrade to b1 (that on main has the same error)
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5027
There was a problem hiding this comment.
Federico Caselli (CaselIT) wrote:
Done
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5027
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision da30dbf of this pull request into gerrit so we can run tests and reviews and stuff
|
Patchset da30dbf added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5027 |
|
Federico Caselli (CaselIT) wrote: recheck seems that the ci test crashed. Let's try restarting it View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5027 |
|
Federico Caselli (CaselIT) wrote: the ci seems happy now. Can you add a changelog? Similar to these ones https://github.com/sqlalchemy/alembic/tree/main/docs/build/unreleased View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5027 |
@CaselIT : done! |
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision dc8c7f8 of this pull request into gerrit so we can run tests and reviews and stuff
|
Patchset dc8c7f8 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5027 |
|
Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5027 has been merged. Congratulations! :) |
Description
When the alembic tree has a migration (a1), with a branched migration (b1) that
depends_onthat migration, followed by a merge migration that merges (a1) and (b1), running the merge migrations downgrade incorrectly updates the heads to [a1, b1], when it should actually just have [b1]. This then prevents a user from running the upgrade again due to the confusing error:The problem occurs in
HeadMaintainer.update_to_stepwhich will update the value of heads by calling out into a helper method based on the scenario: deleting branches, creating branches, merging branches, unmerging branches, or the typical non-branched migration. As it turns out, all of these methods have logic to determine the canonical set of heads that should be written, except in the case we are unmerging, resulting in the redundant head.To fix, we simply remove any ancestors of the target heads from the list of target heads when doing an unmerge.
Fixes #1373
Checklist
This pull request is:
must include a complete example of the issue. one line code fixes without an
issue and demonstration will not be accepted.
Fixes: #<issue number>in the commit messageinclude a complete example of how the feature would look.
Fixes: #<issue number>in the commit messageHave a nice day!