WIP: chore(deps): Upgrade to Django 5.2 by crankynetman · Pull Request #177 · esnet-security/SCRAM

crankynetman · 2025-11-11T20:58:55Z

Well, since it's 2025, I decided to spend down a small nation's amount of energy on Vibe Coding® an upgrade to django 5.2 and python 3.13.

It made a ton of changes I don't understand, but it's less than I figured. And FWIW, the same approaches came about by gemini, claude, and GPT 5o.

I hate this, but the tests do pass locally, so I figured i'd throw this up here for discussion.

…i migrations for django 5

…two places, TIL.

github-actions · 2025-11-12T13:56:14Z

File	Coverage
All files	`80%`	✅
config/consumers.py	`77%`	✅
config/urls.py	`69%`	✅
config/settings/base.py	`69%`	✅
config/settings/local.py	`72%`	✅
scram/route_manager/admin.py	`55%`	✅
scram/route_manager/authentication_backends.py	`83%`	✅
scram/route_manager/models.py	`70%`	✅
scram/route_manager/views.py	`86%`	✅
scram/route_manager/api/serializers.py	`92%`	✅
scram/route_manager/api/views.py	`74%`	✅
scram/shared/shared_code.py	`56%`	✅
scram/templates/403.html	`91%`	✅
scram/templates/404.html	`91%`	✅
scram/templates/base.html	`99%`	✅
scram/templates/route_manager/home.html	`80%`	✅

Minimum allowed coverage is 50%

Generated by 🐒 cobertura-action against e0b02f7

samoehlert · 2025-11-12T15:54:24Z

-          else
-            echo "No unapplied migrations."
-          fi
+          python manage.py migrate --fake-initial --noinput


Why were we able to simplify so much? This seems much less brittle which is great, but are we looking at any side effects? We were conditionally doing this before so was that just not required?

We likely don't need --fake-initial because we've always had migrations be part of things.

I'm still not sure why we have this step at all. Won't migrations always be unapplied on a fresh database? It seems like this was actually originally a "check for migrations that weren't comitted" step which is good, but I think could be rewritten or we could leave it the way it was originally and change the wording.

samoehlert · 2025-11-12T16:00:27Z

 import logging

-from asgiref.sync import sync_to_async
+from channels.db import database_sync_to_async


if my understanding is correct this is basically like some additional ORM aware niceties that channels takes advantage of. this seems like a very good change.

Agreed. We should doublecheck if we can also move to just using the decorator for this, but here's the docs on this, seems like an easy win/drop-in

https://channels.readthedocs.io/en/latest/topics/databases.html#database-sync-to-async

samoehlert · 2025-11-12T16:02:55Z

I get the idea and I'm not opposed to dropping versioning, but on the other hand it seems like it's generally been better for us than just taking anything. Maybe we can find a middle ground and just do major version pinning? This goes for all the requirements files.

We should re-visit this as part of the uv migration (#199 ). I think that moving to ~= X.X (where the X.X is what pip has resolved to and tests pass with) for now in this PR would likely be best.

Maybe we as a short-term don't pin anything, after this merge (or maybe as part of it?) we do the #199 work and rely on the lock file for build reproducibility and then only pin things that we really need pinned? Then do dependabot (#201 ) ASAP?

samoehlert · 2025-11-12T16:07:32Z

Nice this would close (or at least go a long way towards closing) #168

Agreed. I think we probably want to give it a close look but this does seem harmless. We definitely need to merge #188 first though since there are a few changes to migrations over there.

We also need to double-check that we have got rid of index_together everywhere in the codebase.

samoehlert · 2025-11-12T16:11:04Z

I can't see why this is bad, but does it suggest we don't need to use WhoFilter anymore? I think I liked testing WhoFilter directly instead of testing the same thing in a different way. I do think we want to keep it since it's what is used on the admin panel to allow you to filter the list based on the user who added the entry. Maybe I'm overthinking it.

Let's try getting rid of this entire change because I don't think it's needed.

samoehlert · 2025-11-12T16:11:52Z

        self.superuser = get_user_model().objects.create_superuser("admin", "admin@es.net", "admintestpassword")
        self.client.login(username="admin", password="admintestpassword")
+
+        # Create the ActionType


The block actiontype is created by default in one of the early migrations. We can depend on it existing, but maybe this is a case of "better to be explicit"

Maybe this was only needed because of the weird migration change? Worth testing with and without this change to see if it even matters.

Also looking at this more, we should only use get not get_or_create as we'd want this to fail if the migrations aren't there (cuz this is there).

I think we can likely remove this entirely, it's mostly just a bit more readable maybe not really.

samoehlert · 2025-11-12T16:14:24Z

I'm not sure we want this change, but I need to better understand the usage here. password1 and password2 to me suggests a whole password management solution that I don't think we use.

crankynetman · 2026-03-24T17:49:41Z

-          else
-            echo "No unapplied migrations."
-          fi
+          python manage.py migrate --fake-initial --noinput


We likely don't need --fake-initial because we've always had migrations be part of things.

I'm still not sure why we have this step at all. Won't migrations always be unapplied on a fresh database? It seems like this was actually originally a "check for migrations that weren't comitted" step which is good, but I think could be rewritten or we could leave it the way it was originally and change the wording.

crankynetman · 2026-03-24T17:50:36Z

    "channels",
    "corsheaders",
    "crispy_forms",
-    "django_celery_beat",


This is just cleaning up a dangling dependency

crankynetman · 2026-03-24T17:53:05Z

 import logging

-from asgiref.sync import sync_to_async
+from channels.db import database_sync_to_async


Agreed. We should doublecheck if we can also move to just using the decorator for this, but here's the docs on this, seems like an easy win/drop-in

https://channels.readthedocs.io/en/latest/topics/databases.html#database-sync-to-async

crankynetman · 2026-03-24T17:53:28Z

-        routes = await sync_to_async(list)(Entry.objects.filter(is_active=True).values_list("route__route", flat=True))
+        routes = await database_sync_to_async(list)(
+            Entry.objects.filter(is_active=True).values_list("route__route", flat=True)
+        )


That's ruff, buddy

crankynetman · 2026-03-24T18:04:31Z

We should re-visit this as part of the uv migration (#199 ). I think that moving to ~= X.X (where the X.X is what pip has resolved to and tests pass with) for now in this PR would likely be best.

Maybe we as a short-term don't pin anything, after this merge (or maybe as part of it?) we do the #199 work and rely on the lock file for build reproducibility and then only pin things that we really need pinned? Then do dependabot (#201 ) ASAP?

crankynetman · 2026-03-24T18:24:11Z

+from scram.route_manager.models import ActionType, Entry
 from scram.route_manager.models import Client as ClientModel


needs some isort love

crankynetman · 2026-03-24T18:24:53Z

        ]

+        # Create the ActionType
+        self.actiontype, _ = ActionType.objects.get_or_create(name="block")


Same thing here, we should make sure we need this and if so, move to get only.

crankynetman · 2026-03-24T18:25:55Z

        self.unauthorized_user = User.objects.create(username="unauthorized")

-        self.readonly_group = Group.objects.get(name="readonly")
+        # Create the groups that the migration would normally create and assign permissions


Again, likely related to the migration change to pytest. Why do we need to change that? Seems like we want to run migrations before tests and then this should be here already.

crankynetman · 2026-03-24T18:27:13Z



-class TestTranslatorBaseCase(TestCase):
+class TestTranslatorBaseCase(TransactionTestCase):


Probably not needed; probably doesn't hurt.

crankynetman · 2026-03-24T18:30:00Z

            is_admin = groups_overlap(claimed_groups, settings.SCRAM_ADMIN_GROUPS)
            if groups_overlap(claimed_groups, settings.SCRAM_READWRITE_GROUPS):
-                effective_groups.append(Group.objects.get(name="readwrite"))
+                readwrite_group, created = Group.objects.get_or_create(name="readwrite")


Again get vs get_or_create

Okay, let's revert this whole file and then make sure that migrations are a) running appropriately or b) have the right data so we don't do this here, in prod.

crankynetman added 3 commits November 11, 2025 08:41

fix(postgres): use new folder structure.

d6e2d2f

refactor: vibe code a django 5 upgrade because The Future® demands it.

c37cefd

chore(lint): ruffify things

62260c9

crankynetman changed the title ~~chore(deps): Upgrade to Django 5.2~~ WIP: chore(deps): Upgrade to Django 5.2 Nov 11, 2025

crankynetman added 4 commits November 11, 2025 17:55

chore: it seems like django changed migration output a bit.

85de68d

chore(migrations): add migration for the index_together fix and fix c…

5899cd8

…i migrations for django 5

Merge branch 'main' into topic/chriscummings/django-5-upgrade-woohoo

f3bb0e7

chore(deps): we actually have to set python and postgres versions in …

e0b02f7

…two places, TIL.

samoehlert reviewed Nov 12, 2025

View reviewed changes

crankynetman commented May 1, 2026

View reviewed changes

crankynetman marked this pull request as draft May 1, 2026 17:46

crankynetman closed this May 1, 2026

		from scram.route_manager.models import ActionType, Entry
		from scram.route_manager.models import Client as ClientModel



		class TestTranslatorBaseCase(TestCase):
		class TestTranslatorBaseCase(TransactionTestCase):

Conversation

crankynetman commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Nov 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

crankynetman commented Nov 11, 2025 •

edited

Loading