feat: migration from 2.2 -> 3.2 -> 4.2 #2413

JacobCoffee · 2024-03-19T02:17:53Z

What

Attempts to migration from the current 2.2 dependency set to 3.2. Then migrates from 4.2 after fixing any 3.2 issues.
After 4.2 migration, fixes any remaining stale dependencies by finding the first available, noting it in its respective *-requirements.txt, and bumping to the latest available non-breaking version to give "breathing room"
At this point, able to get a live webserver, we can apply migrations and get the loading webpage

Status

Tests passing
This does require a production database migration from PostgreSQL 10 -> 12. I'm not sure what that will entail for the infra team

Todo

~~Does the scope of this include cleaning up the BigAutoField warnings?~~

Auto-created primary key used when not defining a primary key type, by default 'django.db.models.AutoField'.

HINT: Configure the DEFAULT_AUTO_FIELD setting or the $SOMEMODEL.default_auto_field attribute to point to a subclass of AutoField, e.g. 'django.db.models.BigAutoField'.

these dont happen in current version so im going to say yes
resolved via e1ecf70
Verify this is an okay or not change and discuss path forward if not

Does the scope of this include cleaning up the naive datetime warning?

RuntimeWarning: DateTimeField $THING received a naive datetime (2013-08-24 00:00:00) while time zone support is active.

these happen in current version so im going to say no (?)
Opened issue Tests: Silence naive datetime warning spam #2415

Closes

Fixes Upgrade to Django 3.2 LTS version #1860

…t log line

JacobCoffee · 2024-03-19T02:20:22Z

pydotorg/settings/base.py

@@ -163,6 +163,7 @@
    'django.middleware.clickjacking.XFrameOptionsMiddleware',
    'pages.middleware.PageFallbackMiddleware',
    'django.contrib.redirects.middleware.RedirectFallbackMiddleware',
+    'allauth.account.middleware.AccountMiddleware',


right place?

Seems fine to me.

JacobCoffee · 2024-03-19T02:21:41Z

docker-compose.yml

  postgres:
-    image: postgres:10-bullseye
+    image: postgres:12-bullseye  # required for 4.2 upgrades


bullseye vs bookworm? would give us more runway on EOL (bullseye has until 2026, though)

I would prefer bookworm. And postgres:12 is also kind of old - I have postgres:16 on my local machine and it worked without problems.

JacobCoffee · 2024-03-19T02:22:04Z

base-requirements.txt


-django-allauth==0.41.0
+django-allauth==0.61.1  # 0.55.0 is first version that supports Django 4.2


bumped higher than least because of CVE (i think)

JacobCoffee · 2024-03-19T02:22:37Z

base-requirements.txt

-django-markupfield-helpers==0.1.1
+django-honeypot==1.0.4  # 1.0.4 is first version that supports Django 4.2
+django-markupfield==2.0.1
+# django-markupfield-helpers==0.1.1  # this hasnt been moved out of alpha and is last published 2018, we do import it


i cant find where we use this, but it seems we dont. anyway, it doesnt support either 3.2 or 4.2 iirc

JacobCoffee · 2024-03-19T02:23:13Z

base-requirements.txt

-django-pipeline==2.0.6
-django-sitetree==1.17.0
+django-pipeline==3.0.0  # 3.0.0 is first version that supports Django 4.2
+django-sitetree==1.18.0  # >=1.17.1 is (?) first version that supports Django 4.2


unclear changelog on when exactly 4.2 support was introduced

note: unsure if "valid"

JacobCoffee · 2024-03-20T04:01:32Z

Somewhere along the way the email duplication validation from allauth became wonky causing

pythondotorg/users/tests/test_views.py

Line 233 in 9b4dedd

def test_user_duplicate_username_email(self):

to fail.
It seems to be my last failing test, barring i didnt do anything 'bad' by 'fixing' the failing tests post-migration 😅

edit: this was a change in allauth 0.52.0, but disabling it would be a negative since i believe it was added for increased security.

I can add

ACCOUNT_PREVENT_ENUMERATION = False
"""Added to ``django-allauth==0.52.0``. Set during the Django 2.2 -> 4.2 migration."""

into settings.py
or (preferably) add it only to test which seems safer.

def test_user_duplicate_username_email(self):
    """Test that a user can't be created with a duplicate username or email or both."""
    settings.ACCOUNT_PREVENT_ENUMERATION = False

it prevents people from bruteforcing to see which emails are registered
(read more at https://docs.allauth.org/en/latest/account/configuration.html -> search for ACCOUNT_PREVENT_ENUMERATION)

JacobCoffee · 2024-03-20T05:50:56Z

I think this is ready for someone to make fun of me now 🤣, all tests are passing

users/tests/test_views.py

browniebroke

Solid work 💪🏻 I noted a few things. FWIW, I only went though everything you changed (I haven't looked at what you may have missed). Also note that I'm not too familiar with the codebase, but I went through a few Django upgrades, I hope these coments are of use...

browniebroke · 2024-03-20T23:20:57Z

pydotorg/settings/base.py

+DEFAULT_AUTO_FIELD = 'django.db.models.BigAutoField'
+"""The default primary key field type for Django models.
+
+Required during the Django 2.2 -> 4.2 migration.
+"""


I'm not sure about this one, I noticed it generated a bunch of migrations. Should we set it to django.db.models.AutoField while we upgrade and change it after?

Reason I'm asking is that I noticed that the docs mention that not all cases are covered by the migration framework:

Unfortunately, the primary keys of existing auto-created through tables cannot currently be updated by the migrations framework.

This means that if you switch the value of DEFAULT_AUTO_FIELD and then generate migrations, the primary keys of the related models will be updated, as will the foreign keys from the through table, but the primary key of the auto-created through table will not be migrated.

In order to address this, you should add a RunSQL operation to your migrations to perform the required ALTER TABLE step. You can check the existing table name through sqlmigrate, dbshell, or with the field’s remote_field.through._meta.db_table property.

I don't know the codebase too much, so might not be an issue, but it feels like this change in primary key doesn't have to be part of the upgrade (which is quite big already).

docker-compose.yml

events/tests/test_importer.py

browniebroke · 2024-03-20T23:35:36Z

prod-requirements.txt

@@ -3,6 +3,6 @@ gunicorn==19.9.0
 raven==6.10.0


Oh that's an old one 😄 Not sure it supports Django 4.2, but maybe it's fine... Would be nice to see if we can switch to its successor, sentry-sdk.

users/tests/test_forms.py

users/tests/test_views.py

browniebroke · 2024-03-21T09:37:04Z

codesamples/tests.py

+        expected = ['<CodeSample: Copy One>']
+        published_qs = CodeSample.objects.published()
+        actual = [f'<CodeSample: {str(obj)}>' for obj in published_qs]
+        self.assertEqual(actual, expected)


Perhaps this would simpler using assertCountEqual(), like this:

Suggested change

expected = ['<CodeSample: Copy One>']

published_qs = CodeSample.objects.published()

actual = [f'<CodeSample: {str(obj)}>' for obj in published_qs]

self.assertEqual(actual, expected)

self.assertCountEqual(CodeSample.objects.published(),[self.sample2])

I find the name of the method slightly misleading, but it would work nicely here, I think:

Test that sequence first contains the same elements as second, regardless of their order

https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertCountEqual

The only difference I see is that we would no longer cover the __str__ of the model. That could be separate test, if it's deemed important.

It's possible to rewrite those tests like this:

def test_published(self): self.assertQuerySetEqual(CodeSample.objects.published(), ['<CodeSample: Copy One>'], transform=repr)

ephes · 2024-03-25T13:44:15Z

Cool, this really looks good! I didn't see your PR yesterday and started working a bit on the Django update by myself. I also didn't notice that makemigrations would create a lot of migrations, so I got the tests running again without adding those migrations. I honestly don't know whether they are necessary. It's probably a good idea not to fall too much behind, so I think they should be added.

Another thing I did different was trying to update all packages in base and dev-requirements.txt. And except tablib and urllib3 it seems to be possible to upgrade all dependencies.

ephes · 2024-03-25T13:57:25Z

Somewhere along the way the email duplication validation from allauth became wonky causing

pythondotorg/users/tests/test_views.py

Line 233 in 9b4dedd

def test_user_duplicate_username_email(self):

to fail.
It seems to be my last failing test, barring i didnt do anything 'bad' by 'fixing' the failing tests post-migration 😅
edit: this was a change in allauth 0.52.0, but disabling it would be a negative since i believe it was added for increased security.

I can add
ACCOUNT_PREVENT_ENUMERATION = False
"""Added to ``django-allauth==0.52.0``. Set during the Django 2.2 -> 4.2 migration."""
into settings.py or (preferably) add it only to test which seems safer.
def test_user_duplicate_username_email(self):
    """Test that a user can't be created with a duplicate username or email or both."""
    settings.ACCOUNT_PREVENT_ENUMERATION = False
it prevents people from bruteforcing to see which emails are registered (read more at https://docs.allauth.org/en/latest/account/configuration.html -> search for ACCOUNT_PREVENT_ENUMERATION)

The test fails because allauth wont tell you whether an email already exists or not to mitigate against email enumeration. In 0.52.0 there was this setting added, but since 0.55.0 it has no longer an effect because it didn't really work from the start.

But even if no error message is shown to the user, an error message is sent via email to the original user of the email address. What I did was splitting up the test into two to make sure the error message is sent.

cclauss · 2024-03-29T19:42:07Z

Solid work @JacobCoffee !!

A few things you might consider...

Land a smaller PR on this repo so that 1 workflow awaiting approval disappears and GitHub Actions are run on commits.
Perhaps upgrade to 3.2 and let the site run for a few weeks before making a separate upgrade to 4.2.
Add Django to the title: feat: migration from Django 2.2 -> 3.2 -> 4.2.

@ambv Your review, please.

ewdurbin · 2024-04-18T14:26:38Z

Current PG version running python.org's db: 15.3

JacobCoffee · 2024-04-18T17:13:35Z

@ewdurbin is this looking okay so far? If so I can try to finish it up - but I wanted to make sure we wanted to move forward here before continuing anymore work.

ewdurbin · 2024-04-18T17:38:11Z

@JacobCoffee Yes, overall this is looking very promising. I was able to get things stood up in a test environment with a clone of the production DB without any obvious errors.

JacobCoffee added 3 commits March 18, 2024 19:33

feat: initial 2.2 -> 4.2 migration work

ffa4f54

feat: minimum dependency changes required to get a live webserver:por…

6622a65

…t log line

feat: create migrations, apply, and get a loading webpage!

b9c9bb6

JacobCoffee commented Mar 19, 2024

View reviewed changes

JacobCoffee added 9 commits March 19, 2024 13:07

feat(db): squashes todo item: Auto-created primary key used...

e1ecf70

test(minutes): fixed minutes test

29591c1

test(mailing): fixed mailing test

3b984af

test(codesamples): fixed codesamples tests

2583879

test(downloads): fixed downloads tests

83b6505

note: unsure if "valid"

test(pages): fixed pages tests

d23b332

test(successstories): fixed successstories tests

0ae31ce

test(users): fixed users forms tests

2f65efa

test(events): fixed events tests

127dcc9

test(users): fixed users views tests

38f1eff

JacobCoffee marked this pull request as ready for review March 20, 2024 05:50

JacobCoffee commented Mar 20, 2024

View reviewed changes

users/tests/test_views.py Outdated Show resolved Hide resolved

chore: hide my shame

c13ef7a

browniebroke reviewed Mar 20, 2024

View reviewed changes

chore: apply code review

d20fc29

browniebroke reviewed Mar 21, 2024

View reviewed changes

ewdurbin and others added 2 commits April 18, 2024 11:39

harmonize docker-compose and ci.yml PG versions to prod

0f60fc5

Merge branch 'main' into 4.2

f45c2c0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: migration from 2.2 -> 3.2 -> 4.2 #2413

feat: migration from 2.2 -> 3.2 -> 4.2 #2413

JacobCoffee commented Mar 19, 2024 •

edited

JacobCoffee Mar 19, 2024

ephes Mar 25, 2024

JacobCoffee Mar 19, 2024 •

edited

ephes Mar 25, 2024

JacobCoffee Mar 19, 2024

JacobCoffee Mar 19, 2024

JacobCoffee Mar 19, 2024

JacobCoffee commented Mar 20, 2024 •

edited

JacobCoffee commented Mar 20, 2024 •

edited

browniebroke left a comment

browniebroke Mar 20, 2024

browniebroke Mar 20, 2024

browniebroke Mar 21, 2024

ephes Mar 25, 2024 •

edited

ephes commented Mar 25, 2024

ephes commented Mar 25, 2024

cclauss commented Mar 29, 2024 •

edited

ewdurbin commented Apr 18, 2024

JacobCoffee commented Apr 18, 2024

ewdurbin commented Apr 18, 2024


		django-allauth==0.41.0
		django-allauth==0.61.1 # 0.55.0 is first version that supports Django 4.2

feat: migration from 2.2 -> 3.2 -> 4.2 #2413

Are you sure you want to change the base?

feat: migration from 2.2 -> 3.2 -> 4.2 #2413

Conversation

JacobCoffee commented Mar 19, 2024 • edited

What

Status

Todo

Closes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobCoffee Mar 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobCoffee commented Mar 20, 2024 • edited

JacobCoffee commented Mar 20, 2024 • edited

browniebroke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ephes Mar 25, 2024 • edited

Choose a reason for hiding this comment

ephes commented Mar 25, 2024

ephes commented Mar 25, 2024

cclauss commented Mar 29, 2024 • edited

ewdurbin commented Apr 18, 2024

JacobCoffee commented Apr 18, 2024

ewdurbin commented Apr 18, 2024

JacobCoffee commented Mar 19, 2024 •

edited

JacobCoffee Mar 19, 2024 •

edited

JacobCoffee commented Mar 20, 2024 •

edited

JacobCoffee commented Mar 20, 2024 •

edited

ephes Mar 25, 2024 •

edited

cclauss commented Mar 29, 2024 •

edited