-
Notifications
You must be signed in to change notification settings - Fork 31
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
Squash migrations / fix migrations for nautobot 2.0 #113
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.
Is numbering this one as 0101 confusing? Can we number it as something like 0001_0004_0005_0006 instead and would that be clearer?
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.
I'd be concerned about having another 0001_
prefixed file that would be adjacent to 0001_initial
, but I agree with the sentiment of naming the migration more verbosely.
@@ -16,3 +16,13 @@ class Migration(migrations.Migration): | |||
name="site", | |||
) | |||
] | |||
|
|||
def __init__(self, name, app_label): |
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.
Mind adding a docstring or comment explaining this and the similar logic in the previous file for future reference?
dependencies = [ | ||
("nautobot_device_onboarding", "0003_onboardingtask_label"), | ||
("nautobot_device_onboarding", "0007_alter_onboardingtask_ip_address"), | ||
] |
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.
Possibly add a comment here explaining that this is needed to bring the 01->02->03 and the 01->04->05->06->07 branches of the migration tree back together?
I'd also take some suggestions on what we might need to do for the existing release? Should that be yanked or anything? |
Gary can confirm but I don't think the existing release needs to be yanked. We'll just want a new patch release cut ASAP after this fix gets merged. |
This patch will be compatible with the existing release. If someone has tried to upgrade and it failed migration, they should be able to upgrade to the release with these migrations and run migrations successfully. |
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.
🚢 🇮🇹
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.
Looks good to me! Thanks very much for this.
NOTE: This was the result of trying almost every method to get these migrations squashed / working with 2.0 and this was the only option that worked.
We did not squash migrations
0002
or0003
because they contained data migrations that still need to run in 2.0.We also had to make some updates to the existing migrations to fix some circular dependency issues that showed up after the squash. Part of this was adding migration
0008
which should be a no-op for any installs that have already upgraded to 2.0.The
run_before
for the initial migration0001
is no longer necessary because if that migration hasn't run yet, it will be skipped in favor of the squashed migration0101
. So we removed it.We also updated the
run_before
logic for the0004
/0005
migrations to fix circular dependency issues when squashing migrations that containedrun_before