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

chore: Remove django.admin #52679

Merged
merged 2 commits into from
Jul 12, 2023
Merged

chore: Remove django.admin #52679

merged 2 commits into from
Jul 12, 2023

Conversation

markstory
Copy link
Member

We haven't used django-admin in a long time and it was recently disabled in self-hosted (#52329)

Given that we don't use it in saas, and it is not part of any common development workflows I think it is time to not include django-admin in our installed app list.

We haven't used django-admin in a long time and it was recently disabled
in self-hosted (#52329)

Given that we don't use it in saas, and it is not part of any common
development workflows I think it is time to not include django-admin in
our installed app list.
@markstory markstory requested review from a team July 11, 2023 21:37
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 11, 2023
Copy link
Member

@asottile-sentry asottile-sentry left a comment

Choose a reason for hiding this comment

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

@getsantry getsantry bot requested a review from a team as a code owner July 11, 2023 21:44
@dashed
Copy link
Member

dashed commented Jul 11, 2023

@markstory Don't forget to remove this

"django_admin_log",

and to DROP TABLE django_admin_log;

@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Merging #52679 (f099864) into master (2349aaf) will decrease coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #52679      +/-   ##
==========================================
- Coverage   79.37%   79.36%   -0.01%     
==========================================
  Files        4920     4920              
  Lines      206402   206397       -5     
  Branches    35286    35285       -1     
==========================================
- Hits       163823   163817       -6     
- Misses      37555    37559       +4     
+ Partials     5024     5021       -3     
Impacted Files Coverage Δ
src/sentry/conf/server.py 91.71% <ø> (-0.02%) ⬇️
src/sentry/conf/urls.py 100.00% <ø> (+21.42%) ⬆️

... and 7 files with indirect coverage changes

@chadwhitacre
Copy link
Member

chadwhitacre commented Jul 11, 2023

@markstory These are okay?

$ ag 'django.*admin'
CHANGES
3434:  conflict with the Django admin.

src/bitfield/admin.py
1:from django.contrib.admin import FieldListFilter
2:from django.contrib.admin.options import IncorrectLookupParameters

src/social_auth/admin.py
1:from django.contrib import admin

src/sentry/silo/README.md
41:> Could not find silo assignment for django_admin_log

src/sentry/db/router.py
49:        "django_admin_log",

@markstory
Copy link
Member Author

@chadwhitacre I think so. The bitfield and social_auth code won't run unless it is imported, and even if that code is imported, django.contrib.admin is still installed (as it comes with django) we're just no longer including the views/urls in our django application.

@dashed I'll see if I can write a migration to remove that table. I don't think we'll be able to remove the routing line until the next squash migration as the table will be in our migration history until then.

@markstory markstory merged commit 9cd94f1 into master Jul 12, 2023
@markstory markstory deleted the chore-rm-djangoadmin branch July 12, 2023 14:34
@github-actions github-actions bot locked and limited conversation to collaborators Jul 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants