-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Revert stage 2 and 3 of SiteConfiguration/History field rename #24185
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
Revert stage 2 and 3 of SiteConfiguration/History field rename #24185
Conversation
|
Thanks for the pull request, @Agrendalath! I've created OSPR-4679 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:
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. |
|
Your PR has finished running tests. The following contexts failed:
|
|
@Agrendalath Thank you for your contribution. Please let me know once it is ready for our review. |
|
For context, here is a discussion in Slack about the friction in the Juniper upgrade that this PR is trying to fix: https://openedx.slack.com/archives/CSAC4QHC7/p1591672127105600 |
|
Are we still pursuing this? |
pwnage101
left a comment
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.
Based on the conversation in slack that Ned posted, I assume the intention of this PR is to un-do this column rename which currently breaks zero-downtime open edX releases. Since this is not the only column rename, and this also doesn't completely revert the changes for this particular rename (i.e. stage 1 would also need to be reverted for this to make any sense), I would reject it.
|
@nedbat, @pwnage101, thank you for providing the context here. We're closing this, as these breaking changes were done many months ago and reverting them just for Juniper in a forward-compatible way is tricky due to a few previous reverts, plus Juniper is already released, so many could have already upgraded to it. |
|
@Agrendalath Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
Context
The SiteConfiguration and SiteConfigurationHistory models had their
valuesfield renamed tosite_valuesin a few PRs from #22692 to #22753. The testing plan has been defined in Data Engineering docs, however these docs don't mention that each stage should be performed in separate release to avoid downtime or altering business logic between releases, as defined in Architecture and Engineering docs.This is a PoC PR that reverts stage 2 and 3 of #22753. The only conflict was in the
common/djangoapps/util/tests/test_db.pyfile, so these reverts do not seem to be too invasive code-wise. Not sure about any other implications, though. It would be good to get some edX feedback here.Note: This is missing the relevant migrations, so it won't work as-is. We would need to add back the reverted migrations and then create new migrations that would:
valuesfields ofsiteconfigurationandsiteconfigurationhistory.site_valuesfields intovaluesfields.