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

Django 4.2 upgrade for Quince.1 Release #315

Closed
6 of 7 tasks
cmltaWt0 opened this issue Oct 16, 2023 · 19 comments
Closed
6 of 7 tasks

Django 4.2 upgrade for Quince.1 Release #315

cmltaWt0 opened this issue Oct 16, 2023 · 19 comments
Assignees
Milestone

Comments

@cmltaWt0
Copy link

cmltaWt0 commented Oct 16, 2023

List of services and corresponding PRs

Upgrade plan

https://openedx.atlassian.net/wiki/spaces/AC/pages/3767926815/Django+4.2+Upgrade+Plan

Tutor related updates

@iamsobanjaved
Copy link

iamsobanjaved commented Oct 18, 2023

We have upgraded Django on our production instance of course-discovery and here is the backport PR for Quince for course-discovery.

@iamsobanjaved
Copy link

PR to upgrade Django 4.2 in the edx-platform in the Quince release is ready:

@mariajgrimaldi mariajgrimaldi added this to the Quince.1 milestone Oct 24, 2023
@mariajgrimaldi
Copy link
Member

mariajgrimaldi commented Oct 24, 2023

Hi folks. @cmltaWt0: why can we merge the edx-platform upgrade to quince but not master?

@iamsobanjaved
Copy link

Hi folks. @cmltaWt0: why can we merge the edx-platform upgrade to quince but not master?

We have PR (openedx/edx-platform#33211) ready for merging on master, but we are still waiting for MySQL 8.0 upgrade to land on the production instance and also waiting for our sessions to be hashed with the new hashing algorithm. So we wanted to give it first to the Quince release.

Hashing Algorithm Change: edx/edx-arch-experiments#453

More details on Slack: https://openedx.slack.com/archives/C049JQZFR5E/p1697203704251089

@mariajgrimaldi

@cmltaWt0
Copy link
Author

cmltaWt0 commented Oct 25, 2023

@mariajgrimaldi We have Django 3.2 EOL on 01 Apr 2024 - before Redwood so we have to land it in Quince in any case.
I prefer to wait till the master PR be merged.
We can test and merge other PR (for xqueue, discovery) for now.

@mariajgrimaldi
Copy link
Member

I agree with @cmltaWt0. I'd prefer to wait for it to land in master, but I'm worried about the release timeline. We need to have a bit of time to test it out. @iamsobanjaved, do we have an estimate for merging the masters' PR? I'm also thinking we could benefit from the update early on in the testing process.

@iamsobanjaved
Copy link

We don't yet have any timeline for MySQL 8.0 upgrade on prod and hence blocked on it, there is an active effort on upgradation but facing certain issues. Also, we might have to wait for at least two weeks for old sessions to be replaced with new sessions. We have added all support commits on edx-platform master before branch cut and now only this last PR is remaining.

My rationale for merging it into the release branch as soon as possible is that Open edX conducts rigorous testing on this branch. This way, if there are any errors, they can be detected during testing, and we can promptly provide fixes for them.

But it is up to you guys to decide what fits for you.

@mariajgrimaldi
Copy link
Member

As I said, I usually agree with waiting for the master merge. But we should test this as soon as possible to identify any issues. @cmltaWt0 what do you think?

@cmltaWt0
Copy link
Author

cmltaWt0 commented Oct 26, 2023

Regarding the current state I can agree with the reasoning to merge it to the release branch so we can catch any weird behaviour.
Let's at least build the openedx image from the source branch and run locally before merge.

@mariajgrimaldi
Copy link
Member

Great! Thank you both

@cmltaWt0
Copy link
Author

Just tested it.
It looks like we can't merge it without tutor preparation for the upgrade.

tutor local launch gives me an Import error on this line.

from django.utils.deprecation import RemovedInDjango40Warning, RemovedInDjango41Warning
File "/openedx/edx-platform/lms/envs/tutor/production.py", line 156, in <module>
    from django.utils.deprecation import RemovedInDjango40Warning, RemovedInDjango41Warning
ImportError: cannot import name 'RemovedInDjango40Warning' from 'django.utils.deprecation' (/openedx/venv/lib/python3.8/site-packages/django/utils/deprecation.py)

@cmltaWt0
Copy link
Author

cmltaWt0 commented Oct 26, 2023

It seems the process may be more complicated :)

ERRORS:
?: (4_0.E001) As of Django 4.0, the values in the CSRF_TRUSTED_ORIGINS setting must start with a scheme (usually http:// or https://) but found apps.local.overhang.io. See the release notes for details.

So we have to update tutor-mfe quince as well (and probably any other IDA plugins).

@mariajgrimaldi
Copy link
Member

Pff. Thanks for testing that out! Should we open an issue in the tutor repository for the migration? So everyone is in sync.

@cmltaWt0
Copy link
Author

Yeap. Definitely.

@cmltaWt0
Copy link
Author

cmltaWt0 commented Oct 26, 2023

I've manage to build and run the Django 4.2 for lms/cms and mfe container.
It works from the first look. Will create PRs for tutor and tutor-mfe.

@iamsobanjaved
Copy link

iamsobanjaved commented Oct 26, 2023

Just tested it. It looks like we can't merge it without tutor preparation for the upgrade.

tutor local launch gives me an Import error on this line.

from django.utils.deprecation import RemovedInDjango40Warning, RemovedInDjango41Warning
File "/openedx/edx-platform/lms/envs/tutor/production.py", line 156, in <module>
    from django.utils.deprecation import RemovedInDjango40Warning, RemovedInDjango41Warning
ImportError: cannot import name 'RemovedInDjango40Warning' from 'django.utils.deprecation' (/openedx/venv/lib/python3.8/site-packages/django/utils/deprecation.py)

You can replace with Django50 and 51 warnings.

It seems the process may be more complicated :)

ERRORS:
?: (4_0.E001) As of Django 4.0, the values in the CSRF_TRUSTED_ORIGINS setting must start with a scheme (usually http:// or https://) but found apps.local.overhang.io. See the release notes for details.

So we have to update tutor-mfe quince as well (and probably any other IDA plugins).

Yes we need this change, also mentioned in the Quince release notes here: https://openedx.atlassian.net/wiki/spaces/COMM/pages/3726802953/Pending+Release+Quince#Operational-%E2%9A%99%EF%B8%8F

@mariajgrimaldi
Copy link
Member

@iamsobanjaved: did you folks consider this change when upgrading to Django 4.x? #325 (comment)

@iamsobanjaved
Copy link

Yes we were aware of this change but we already had our host in CSRF_TRUSTED_ORIGINS so we just had to add the scheme, added this to Quince Release here

@mariajgrimaldi
Copy link
Member

@iamsobanjaved: so you folks no only add the MFE domains but all subdomains, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

3 participants