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

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

Closed
wants to merge 18 commits into from
Closed

Conversation

JacobCoffee
Copy link
Member

@JacobCoffee JacobCoffee commented Mar 19, 2024

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.

Closes

docker-compose.yml Outdated Show resolved Hide resolved
base-requirements.txt Outdated Show resolved Hide resolved
@JacobCoffee
Copy link
Member Author

JacobCoffee commented Mar 20, 2024

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

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 JacobCoffee marked this pull request as ready for review March 20, 2024 05:50
@JacobCoffee
Copy link
Member Author

JacobCoffee commented Mar 20, 2024

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

Copy link

@browniebroke browniebroke left a comment

Choose a reason for hiding this comment

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

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...

pydotorg/settings/base.py Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
events/tests/test_importer.py Outdated Show resolved Hide resolved
@@ -3,6 +3,6 @@ gunicorn==19.9.0
raven==6.10.0

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
users/tests/test_forms.py Outdated Show resolved Hide resolved
users/tests/test_views.py Outdated Show resolved Hide resolved
@ephes
Copy link

ephes commented Mar 25, 2024

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
Copy link

ephes commented Mar 25, 2024

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

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
Copy link
Contributor

cclauss commented Mar 29, 2024

Solid work @JacobCoffee !!

A few things you might consider...

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

@ambv Your review, please.

@ewdurbin

This comment was marked as resolved.

@JacobCoffee
Copy link
Member Author

@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
Copy link
Member

@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.

Co-authored-by: Ezio Melotti <ezio.melotti@gmail.com>
@JacobCoffee
Copy link
Member Author

In favor of #2520

@JacobCoffee JacobCoffee closed this Sep 4, 2024
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.

Upgrade to Django 3.2 LTS version
6 participants