-
Notifications
You must be signed in to change notification settings - Fork 14k
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 order due to cherry which went astray #26160
fix: Migration order due to cherry which went astray #26160
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
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #26160 +/- ##
=======================================
Coverage 69.20% 69.20%
=======================================
Files 1941 1941
Lines 75906 75904 -2
Branches 8457 8457
=======================================
Hits 52533 52533
+ Misses 21177 21175 -2
Partials 2196 2196
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
4612425
to
e05e760
Compare
) | ||
insp = reflection.Inspector.from_engine(engine) | ||
|
||
insp = inspect(op.get_context().bind) |
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 table_has_column
method was returning False
for PostgreSQL within CI even though the columns existed. The proposed update is from the same Stack Overflow post, however it's from 2022 (as opposed to 2018) and has more votes.
Thanks @john-bodley, this looks good to me too. I outlined what the "after" would look like as I was reading through this, so pasting it here for anyone else interested. (pls correct me if I missed anything) master
3.0
3.1.0
3.1.1
|
Thanks @eschutho for adding this—I was wondering whether I should have added something similar. I did update your comment to include |
(cherry picked from commit 8644b1a)
Great, I updated the comment for anyone else reading this, edited it to 3.1.0 specifically so we can see where it was added in later, and I showed where the |
(cherry picked from commit 8644b1a)
SUMMARY
This PR fixes a Alembic migration conundrum when upgrading from
3.0
to3.1
due to a picked cherry which somehow went astray.The Alembic visions chain on the various branches is:
master
ec54aca4c8a2
317970b4400c
4b85906e5b91
3.0
ec54aca4c8a2
4b85906e5b91
3.1
ec54aca4c8a2
317970b4400c
4b85906e5b91
i.e., per the previously mentioned cherry in 3.0 revision
4b85906e5b91
somehow became rewired from317970b4400c
toec54aca4c8a2
meaning the317970b4400c
was never present in 3.0.This PR:
317970b4400c
.b7851ee5522f
—which should also be cherry-picked into3.1
—which merely replays317970b4400c
for bothmaster
and3.1
.These two steps ensure that if your upgrading/downgrading from either
master
or3.0
that317970b4400c
will run if and only if the migration has not been run previous—determined by the presence/absence of the column respectively.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
CI.
ADDITIONAL INFORMATION