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

Support Django 4 #2801

Merged
merged 8 commits into from
Aug 21, 2022
Merged

Support Django 4 #2801

merged 8 commits into from
Aug 21, 2022

Conversation

ivarnakken
Copy link
Member

@ivarnakken ivarnakken commented Jul 26, 2022

The future is now

I've migrated what seems to be necessary in order to support Django 4. But, the new version brings a bunch of new interesting features that I would like to have a closer look at (potentially in a new PR). Nevertheless, this should be "ready" to review.

djangorestframework-jwt is replaced with drf-jwt, which is simply an "updated" fork. However, I think djangorestframework-simplejwt has some cool features (and the author of djangorestframework-jwt agrees), so I naturally begun to migrate to it but realized in the midst that it was kinda out of scope of what I'm trying to achieve with this PR, so drf-jwt will have to do for now - but will be replaced soon! Also, it's nice to diversify breaking changes into multiple PRs to minimize risk 😅

@ivarnakken ivarnakken added in-progress future-is-now Pull requests that utilize cutting-edge technology or just a cool new feature labels Jul 26, 2022
@ivarnakken ivarnakken force-pushed the django-4 branch 6 times, most recently from e34f744 to 4359835 Compare July 26, 2022 11:58
@ivarnakken ivarnakken added review-needed Pull requests that need review testing-needed Pull requests that need testing, e.g. stress tests by users or specs do-not-merge/hold Pull request is on hold (e.g. waiting for something else to be merged), and should not be merged labels Jul 26, 2022
@ivarnakken ivarnakken marked this pull request as ready for review July 26, 2022 12:08
@ivarnakken ivarnakken force-pushed the django-4 branch 5 times, most recently from 64a1827 to 44257e0 Compare July 26, 2022 18:01
Copy link
Contributor

@erlingfn erlingfn left a comment

Choose a reason for hiding this comment

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

The code looks good, however there seems to be a few errors with authentication still in

@ivarnakken ivarnakken added do-not-merge/WIP Pull requests that are "work in progress", and should not be merged and removed do-not-merge/hold Pull request is on hold (e.g. waiting for something else to be merged), and should not be merged labels Jul 28, 2022
Copy link
Member

@LudvigHz LudvigHz left a comment

Choose a reason for hiding this comment

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

Haven't looked through the change notes to check if anything is missing. But just from looking at the changes here everything looks good!

missing-migrations runs fine locally, so not quite sure why it fails in CI though 🤔

@ivarnakken
Copy link
Member Author

[...], however there seems to be a few errors with authentication still in

@erlingfn. The auth problems are fixed in webkom/lego-webapp#2924

@ivarnakken
Copy link
Member Author

ivarnakken commented Aug 2, 2022

From this related Stack Overflow question I'm suspecting that missing-migrations is failing in CI because the db is not fully ready during the first migration, yet Drone is trying to access the db. Does this sound like a possible reason?

I recommend running Drone locally for a better traceback of the error: drone exec .drone.yml --pipeline default --event push --include=setup --include=database --include=missing-migrations

EDIT: Issue is resolved

@ivarnakken ivarnakken removed do-not-merge/WIP Pull requests that are "work in progress", and should not be merged in-progress labels Aug 3, 2022
@ivarnakken ivarnakken force-pushed the django-4 branch 5 times, most recently from 6f07760 to f0353b5 Compare August 13, 2022 22:17
@codecov-commenter
Copy link

codecov-commenter commented Aug 13, 2022

Codecov Report

Merging #2801 (538a49b) into master (8303941) will increase coverage by 0.00%.
The diff coverage is 92.85%.

@@           Coverage Diff           @@
##           master    #2801   +/-   ##
=======================================
  Coverage   78.93%   78.93%           
=======================================
  Files         319      319           
  Lines       10709    10711    +2     
=======================================
+ Hits         8453     8455    +2     
  Misses       2256     2256           
Impacted Files Coverage Δ
lego/apps/websockets/routing.py 0.00% <0.00%> (ø)
lego/api/urls.py 100.00% <100.00%> (ø)
lego/apps/events/notifications.py 48.93% <100.00%> (ø)
lego/apps/events/serializers/registrations.py 100.00% <100.00%> (ø)
lego/apps/jwt/handlers.py 100.00% <100.00%> (ø)
lego/apps/meetings/notifications.py 83.33% <100.00%> (ø)
lego/apps/oauth/urls.py 100.00% <100.00%> (ø)
lego/urls.py 85.71% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ivarnakken ivarnakken added dependencies Pull requests that update a dependency file do-not-merge/hold Pull request is on hold (e.g. waiting for something else to be merged), and should not be merged labels Aug 13, 2022
ivarnakken added a commit to webkom/lego-webapp that referenced this pull request Aug 15, 2022
`drf-jwt` requires Bearer, instead of `djangorestframework-jwt`'s JWT scheme. This is related to webkom/lego#2801
@ivarnakken
Copy link
Member Author

I've done some extensive testing with this in staging, and everything seems to work as intended ™️. Would appreciate if others had a go with is as well ❤️

The Python standard library’s zoneinfo is now the default timezone implementation in Django:
https://docs.djangoproject.com/en/4.0/releases/4.0/#zoneinfo-default-timezone-implementation
drf-jwt is an "updated" fork of djangorestframework-jwt, and give support for Django 4. Recommended by the author of djangorestframework-jwt: jpadilla/django-rest-framework-jwt#484
url() was deprecated in Django 3, and removed in Django 4.
As well as django-push-notifications to v3.0.0, in order to support Django 4.
E.g. 'coverage' and 'debug-toolbar' was missing when running `missing-migrations` through tox.
Required by Django
These were causing `missing_migrations` to fail.
@ivarnakken ivarnakken added approved Pull requests that have been approved ready-to-merge Pull requests that have been approved and are ready to be merged and removed do-not-merge/hold Pull request is on hold (e.g. waiting for something else to be merged), and should not be merged review-needed Pull requests that need review labels Aug 21, 2022
@ivarnakken ivarnakken merged commit 84fc5e7 into master Aug 21, 2022
@ivarnakken ivarnakken deleted the django-4 branch August 21, 2022 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Pull requests that have been approved dependencies Pull requests that update a dependency file future-is-now Pull requests that utilize cutting-edge technology or just a cool new feature ready-to-merge Pull requests that have been approved and are ready to be merged testing-needed Pull requests that need testing, e.g. stress tests by users or specs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants