Skip to content

Conversation

@awais786
Copy link
Contributor

@awais786 awais786 commented Sep 16, 2025

Pre-merge Checklist

  • Create a sandbox
  • Create a new course in Studio and test import/export
  • Attempt a purchase to verify payment flow
  • Do so some progress in a course as student.
  • Run migrate command to make sure no hidden migrations appear.

NOTE:

Previously, edx-platform was usually the last service to upgrade to Django 5.2, which meant most issues were already discovered and resolved elsewhere. This time, however, it is the first service moving to Django 5.2, so we may need to wait for credential and discovery updates before proceeding.

@awais786 awais786 added the create-sandbox A test sandbox will be created for this PR, using the `open-craft/pr-sandbox-automation` tool label Sep 16, 2025
# python3-openid
# social-auth-core
django==4.2.24
django==5.2.6
Copy link
Contributor Author

@awais786 awais786 Sep 16, 2025

Choose a reason for hiding this comment

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

Version upgraded. Now this is pinned version.

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@awais786 awais786 requested review from feanil and timmc-edx October 3, 2025 10:30

# Date: 2024-02-02
# Stay on LTS version, remove once this is added to common constraint
Django<5.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be updated to Django<6.0 rather than being removed?

Copy link
Member

Choose a reason for hiding this comment

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

Updated the constraint to Django<6.0 instead.

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@UsamaSadiq UsamaSadiq marked this pull request as ready for review October 7, 2025 05:31
@UsamaSadiq
Copy link
Member

@robrap could you help us create a 2U sandbox to test this PR?

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@robrap
Copy link
Contributor

robrap commented Oct 7, 2025

Is the OpenCraft sandbox not working? We have a pipeline/sandbox issue at this time.

@UsamaSadiq
Copy link
Member

Is the OpenCraft sandbox not working? We have a pipeline/sandbox issue at this time.

Above logs show the open craft sandbox failing but the error logs not show any error in it.

@robrap
Copy link
Contributor

robrap commented Oct 7, 2025

It feels like the sandbox build can be flaky. The message has the following instructions:

Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.

Maybe it will build fine after a second try?

@feanil
Copy link
Contributor

feanil commented Oct 7, 2025

Looks like the issue is with Tutor: https://openedx.slack.com/archives/C05519HHZKM/p1759846814275579?thread_ts=1759845987.738659&cid=C05519HHZKM

So I think it's fine to merge this and that can be fixed upstream.

@robrap
Copy link
Contributor

robrap commented Oct 7, 2025

@feanil @awais786

  • It makes me nervous to have an approved PR with breaking changes, and no "[DO NOT MERGE]" in the title.
  • Is the plan to merge this after the Ulmo cut? 2U is intending to be on forks with Ulmo, but we are still deploying master today.

@feanil
Copy link
Contributor

feanil commented Oct 7, 2025

@robrap
Copy link
Contributor

robrap commented Oct 7, 2025

@UsamaSadiq: Our pipeline and sandbox builds are still broken. Hopefully that will be resolved in the next day or so, if you would still like a 2U sandbox as well.

@robrap
Copy link
Contributor

robrap commented Oct 7, 2025

@feanil @UsamaSadiq: Also, I assume all plugins need to be Django 5.2 compatible by the time this is merged, because edx-platform is what will define the Django version. Is that correct? I don't think we have a breaking change ticket and date filed, so maybe we can just handle this post-Ulmo?

@robrap robrap changed the title feat!: Upgrading to django52. [DO NOT MERGE] feat!: Upgrading to django52. Oct 7, 2025
@feanil
Copy link
Contributor

feanil commented Oct 7, 2025

That's already done, all python libraries in the openedx org already test with Django 4.2 and 5.2

@feanil
Copy link
Contributor

feanil commented Oct 7, 2025

Filed a fix upstream: overhangio/tutor#1282

@robrap
Copy link
Contributor

robrap commented Oct 7, 2025

[inform] I added [DO NOT MERGE] to the title for now, until we can discuss. We've had several recent miscommunications where PRs have been merged that should not have been. This will soon be a problem of the past. Thank you.

@feanil
Copy link
Contributor

feanil commented Oct 7, 2025

@robrap I don't think that's correct here, I was about to merge this. It's been well communicated: openedx/public-engineering#339

All libraries have been updated and testing is passing.

@feanil feanil changed the title [DO NOT MERGE] feat!: Upgrading to django52. feat!: Upgrading to django52. Oct 7, 2025
@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@robrap
Copy link
Contributor

robrap commented Oct 7, 2025

@feanil: Here are some thoughts:

  • We've all seen Django upgrade reverts, so this is definitely a risky change, but hasn't had the merge warned in risky-changes, nor has there been a DEPR with the breaking change date.
  • I agree that I knew Django 5.2 was being worked on, but did not know that you were moments away from trying to merge, nor when we were trying to merge.
  • I understand that getting this change in before Ulmo means you get to have it fully tested as part of Ulmo. So I get that this would be your preference.
  • Although everything on openedx has been tested, it doesn't mean that all orgs have tested all of their plugins, etc. This is still a breaking change that may affect the community in ways you can't test for.
  • 2U will soon no longer be an early deployer. I'm only stating this because we can see the light at the end of the tunnel, and hopefully this will be one of the last of these types of discussions.

Given all of this, can you hold back for a moment so I can discuss with my team and figure out a game plan?

@feanil
Copy link
Contributor

feanil commented Oct 7, 2025

Sure, how long do you need?

@robrap
Copy link
Contributor

robrap commented Oct 7, 2025

@feanil: I'll get back to you in an hour. I presume you are going to merge (soon) and we are just going to have to find a way to deal with it on our end, but this will give me a moment to consider possibilities. Thank you.

@feanil
Copy link
Contributor

feanil commented Oct 7, 2025

Happy to wait if you have specific things you want to check and a timeline to check them on. I was less okay with a general do-not-merge with no plan for who would be responsible for following up on that. Especially since, operators should be fully aware that this was coming and we've had plenty of places where folks can follow progress. Let me know how much time you need and when you expect to have more info.

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@robrap
Copy link
Contributor

robrap commented Oct 7, 2025

@feanil: Thanks for your patience. You are clear for take off.

@feanil feanil merged commit d484910 into master Oct 7, 2025
49 checks passed
@feanil feanil deleted the final-dj52 branch October 7, 2025 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

create-sandbox A test sandbox will be created for this PR, using the `open-craft/pr-sandbox-automation` tool

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants