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

fix: sql_require_primary_key compatibility #116

Merged

Conversation

gabor-boros
Copy link

In MySQL, if the sql_require_primary_key is turned on, no table can
exist without a primary key. Since the migration 0005 drops the primary
key field before model deletion, the table misses the primary key, hence
the migration will fail.

To prevent migration failure, we swap the primary key before dropping
the revisionpluginrevision_ptr field which is the actual primary key.
In case the migration need to be reverted, the opposite will happen and
we set the revisionpluginrevision_ptr as the primary key.

When selecting the new primary key, it doesn't really matter what we
set, since at the end of the migration we drop the whole table.

Signed-off-by: Gabor Boros gabor.brs@gmail.com

In MySQL, if the `sql_require_primary_key` is turned on, no table can
exist without a primary key. Since the migration 0005 drops the primary
key field before model deletion, the table misses the primary key, hence
the migration will fail.

To prevent migration failure, we swap the primary key before dropping
the `revisionpluginrevision_ptr` field which is the actual primary key.
In case the migration need to be reverted, the opposite will happen and
we set the `revisionpluginrevision_ptr` as the primary key.

When selecting the new primary key, it doesn't really matter what we
set, since at the end of the migration we drop the whole table.

Signed-off-by: Gabor Boros <gabor.brs@gmail.com>
@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Apr 11, 2022
@openedx-webhooks
Copy link

Thanks for the pull request, @gabor-boros! I've created OSPR-6603 to keep track of it in JIRA, where we prioritize reviews. 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.

@gabor-boros
Copy link
Author

@natabene who should be contacted for a review here? 😊

@natabene
Copy link

@gabor-boros Thank you for your contribution. @jmyatt Could you review this OSPR when you have a chance?

@gabor-boros
Copy link
Author

@natabene @jmyatt May I help with any additional info for the review process?

@gabor-boros
Copy link
Author

@UsamaSadiq may could help out too with reviewing 😊

@UsamaSadiq
Copy link
Member

@gabor-boros I'll prioritise it for review within the team soon. Thanks for the PR.

@gabor-boros
Copy link
Author

Thank you @UsamaSadiq!

Copy link
Member

@UsamaSadiq UsamaSadiq left a comment

Choose a reason for hiding this comment

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

@gabor-boros thanks for your patience. I'll go ahead and merge this PR as well.

@natabene
Copy link

@UsamaSadiq Excited about this merge.

@UsamaSadiq UsamaSadiq merged commit 0a1d555 into openedx:edx_release Jun 17, 2022
@openedx-webhooks
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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