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

Update Django to v3.2 #625

Merged
merged 18 commits into from
Jul 18, 2023
Merged

Update Django to v3.2 #625

merged 18 commits into from
Jul 18, 2023

Conversation

jarosenb
Copy link
Contributor

@jarosenb jarosenb commented Apr 14, 2023

Overview

Update Django to v3.2 and bump deprecated dependencies.

Related

Testing

  1. Spin up the CMS like normal and add/publish/delete pages.

@jarosenb jarosenb changed the title Update to Django 3.2 Update Django to v3.2 Apr 14, 2023
@wesleyboar wesleyboar self-requested a review April 21, 2023 20:51
@wesleyboar
Copy link
Member

The changelog of updated plugins I reviewed1 look safe (and sometimes great).

Footnotes

  1. I reviewed those that I installed or that CMD relies on heavily. I skipped asgiref and django-appdata and django-meta, because they are used by other plugins I did review updates to.

@wesleyboar
Copy link
Member

Copy link
Member

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

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

After completing the rename of example-cms to example_cms1, I came across a follow-up problem. If we do this on main, then every website must have its app name renamed from aaa-bbb to aaa_bbb. I say this because I got this error:

[...] django.core.exceptions.ImproperlyConfigured: The app label 'a2cps-cms' is not a valid Python identifier.

And I expect to get that error for every directory in https://github.com/TACC/Core-CMS-Resources. Renaming these also means renaming template paths2 for some websites, which means (upon re-deploy of those websites) re-assigning the template each page uses—a gotcha likely to be forgotten or neglected or ignored, causing failure and delay with deploy.

Due to the priority given to this task, I feel forced to propose this PR be built off of dev/tup-cms instead of main.

Footnotes

  1. Because a dash is not a valid Python identifier, and now Django prevents such a name.

  2. Because template paths (used to load custom assets) include the app directory name.3

  3. Because template overriding is not supported in Core-CMS-Resources.45

  4. Because I created project customization without enough knowledge.

  5. An old, paused attempt to fix this is Bugfix/fp 1652 template load order #492.

@wesleyboar
Copy link
Member

I'll be back to work on this, next week.

@wesleyboar
Copy link
Member

Requires TACC/Core-CMS-Resources#176.

Because fixing other sites will slow down TUP work, we've created #626.

@wesleyboar wesleyboar added the paused Started but not actively in progress label Apr 24, 2023
@wesleyboar
Copy link
Member

wesleyboar commented May 1, 2023

I deleted my unnecessary message about being ready to test on dev.

Testing happens off of #626 not (not #625).

@taoteg
Copy link
Collaborator

taoteg commented May 11, 2023

Just wanted to add that there appear to be some versioning mismatches here, in that DjangoCMS v3.7.4 only supports up to Django v3.0.

Current versions:

  • DjangoCMS 3.7.4 --> Django 2.2.27 (Can support up to Django 3.0)

Updates since:

  • DjangoCMS 3.8.0 --> Django 3.1
  • DjangoCMS 3.9.0 --> Django 3.2 LTS
  • DjangoCMS 3.11.1 --> Django 4.1
  • DjangoCMS 3.11.3 --> Django 4.2 LTS. [Supported until End of Life in April 2026]

https://docs.django-cms.org/en/latest/upgrade/index.html

@wesleyboar
Copy link
Member

@jgentle Uh oh. And thank you.

So, Django 3.2 + Django v3.8.0 = incompatible?

We released the upgrade on TUP via #626. I recorded (CMS?) admin UI bugs in TUP-493.

@taoteg
Copy link
Collaborator

taoteg commented May 12, 2023

I don't know about incompatible, but not fully supported for sure. I would expect there would be things that break, else they would have made the dependency jump in the CMS. Its probably a case-by-case basis on how well the compatibility maps, so may depend on what pieces of the CMS we are using in terms of seeing breakage. But it isn't officially supported so if stuff does break for us, that would be on us to deal with if we don't migrate to a newer version.

Do we have any overt blockers if we jump to the front of the current releases line and use DjangoCMS 3.11.3 --> Django 4.2 LTS. [Supported until End of Life in April 2026]?

(aside from working through the version bumps on the dependencies that would entail... and of course thar be dragons hidden here too).

Thoughts?

@wesleyboar
Copy link
Member

wesleyboar commented May 12, 2023

@taoteg Okay, thanks.

I bet that explains the admin UI bugs I found. Otherwise, the admin UI works, and the rendering has not changed.

Yes. Jump delayed until TUP-454 because Jake found plugins that may not survive the jump.

@wesleyboar
Copy link
Member

wesleyboar commented May 24, 2023

To Do

  1. Merge main and resolve taccsite_custom conflict.
    I think it should match latest of TACC/Core-CMS-Resources#176's branch.
  2. Merge main and resolve poetry.lock conflict.
  3. Merge main and resolve new taccsite_custom and Poetry dep's conflict.
  4. Test a non-TACC websites with the upgrade.
  5. → Test feat!: rename directories, "-" → "_" (for django 3) Core-CMS-Resources#176

@wesleyboar wesleyboar mentioned this pull request Jun 9, 2023
@wesleyboar
Copy link
Member

wesleyboar commented Jun 12, 2023

I've noticed many warnings, in TUP-CMS, which has this upgrade.

I reported them to the source app generating the most: nephila/djangocms-blog#737.

A solution could be in our control. The problem might be because of the incompatibility @taoteg found.

Uncertain.

@wesleyboar
Copy link
Member

I indirectly tested this on dev.cep successfully by deploying v3.12.0-alpha.1 (a.k.a. dev/tup-cms / #581).

@wesleyboar
Copy link
Member

This PR was essentially merged, because #581 was merged (earlier than I expected), which included #626, which is exactly this.

@wesleyboar wesleyboar removed the paused Started but not actively in progress label Jul 18, 2023
@wesleyboar
Copy link
Member

@wesleyboar wesleyboar self-requested a review July 18, 2023 21:19
@wesleyboar wesleyboar merged commit 009af84 into main Jul 18, 2023
@wesleyboar wesleyboar deleted the django/2to3 branch July 18, 2023 21:20
@wesleyboar
Copy link
Member

@taoteg Regarding your concern about question, I think we jumped further along than you think. Our use of ^ updates minor version, not just patch version, so:

This PR (actually #581) took us from Django CMS 3.7.4 to 3.11.1 and from Django 2.2.27 to 3.2.18.

And a new PR (#707) takes us from Django CMS 3.11.3 to 3.11.4 and from Django 3.2.19 to 4.2.5.

My brain is thoroughly scrambled now. Please help. Does this resolve your concern?

@taoteg
Copy link
Collaborator

taoteg commented Sep 20, 2023

Sounds like we are going to land at DjangoCMS 3.11.4 --> Django 4.2.5 (LTS) when this is all merged in. This does resolve my concerns. Nice work!

@wesleyboar
Copy link
Member

Credit all to @jarosenb for the hard work. I just read and test and note.

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

Successfully merging this pull request may close these issues.

3 participants