Skip to content
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 Tagging Migrations [FC-0030] #95

Merged
merged 2 commits into from
Oct 10, 2023
Merged

Conversation

bradenmacdonald
Copy link
Contributor

@bradenmacdonald bradenmacdonald commented Oct 10, 2023

This squashes all the migrations from 0001 through 0011 into a single file. The old migrations are kept in place for now, per best practice, but they are unused and can soon be deleted.

Once openedx/edx-platform#33438 is merged, the only migration that edx-platform refers to will be 0012_language_taxonomy which is kept unchanged as an un-squashed migration. This is nice because it means that after we merge this, we don't have to make any corresponding changes to edx-platform; it won't have any references to any of the squashed migrations. However, we would have to merge a version bump PR to make sure this version of openedx-learning with the squashed PRs is used. I believe the Quince cutoff is scheduled for tomorrow, but I'm hoping we can get this in on time as having the squashed migrations will make things easier to reason about.

Why I want to squash the migrations: one of the migrations loaded data from a fixture, which I realize now is a bad practice as it means that changes to the fixture file are not reflected in the migration. In this case we don't even need the fixture any more but deleting it and changing the migration accordingly turned out to produce a string of minor whack-a-mole problems. Likewise, one of the migrations included an import of a mixin that is now deleted, and so the migration actually failed even though we didn't modify it. So we had to change an existing migration just to avoid a bug on new installations of this repo. To resolve all these things cleanly and make things easier, it's best to just squash the migrations.

Once this PR is merged, the tagging app will essentially have only two migrations: 0001_squashed and 0012_language_taxonomy. Future migrations will start with 0013 and proceed from there. But for the time being, we keep copies of the unsquashed migration files until we're sure that nobody has an intermediate state deployed. We can delete them in a week or two.

Private-ref: FAL-3477

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 10, 2023
@openedx-webhooks
Copy link

Thanks for the pull request, @bradenmacdonald! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@bradenmacdonald bradenmacdonald changed the title Squash Tagging Migrations Squash Tagging Migrations [FC-0030] Oct 10, 2023
Copy link
Contributor

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 This works fine, thank you @bradenmacdonald !

I recommend merging this and tagging a new release and using 0.2.3 in openedx/edx-platform#33438, so that all of this makes it into Quince.

  • I tested this by applying and rolling back migrations on the devstack as described here.
  • I read through the code
  • I checked for accessibility issues N/A
  • Includes documentation
  • Commit structure follows OEP-0051

@bradenmacdonald bradenmacdonald merged commit 8ba0043 into main Oct 10, 2023
6 checks passed
@bradenmacdonald bradenmacdonald deleted the braden/squash-migrations branch October 10, 2023 06:56
@openedx-webhooks
Copy link

@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@itsjeyd itsjeyd added the core contributor PR author is a Core Contributor (who may or may not have write access to this repo). label Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core contributor PR author is a Core Contributor (who may or may not have write access to this repo). open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants