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

task/WP-271 Upgrade Django to 4.2 and update other 3rd party dependencies #895

Merged
merged 15 commits into from
Nov 21, 2023

Conversation

shayanaijaz
Copy link
Contributor

@shayanaijaz shayanaijaz commented Nov 6, 2023

Overview

Updates Django to version 4.2, addresses deprecation warnings and any breaking changes.

Resources:

Related

Changes

  • Updated django version and other dependancies that were throwing deprecation errors

Testing

  1. Go to /server and run poetry install
  2. Run make build
  3. Run migrations
  4. Ensure everything works as expected, monitor docker logs for any errors thrown
  5. Run server testing and make sure no warning messages are shown

UI

Notes

Added comments for some stuff I am not sure about so would appreciate some feedback on that

Copy link

codecov bot commented Nov 6, 2023

Codecov Report

Merging #895 (31448ec) into main (c97ce0c) will increase coverage by 0.05%.
The diff coverage is 57.14%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #895      +/-   ##
==========================================
+ Coverage   63.40%   63.46%   +0.05%     
==========================================
  Files         432      432              
  Lines       12404    12406       +2     
  Branches     2585     2585              
==========================================
+ Hits         7865     7873       +8     
+ Misses       4327     4317      -10     
- Partials      212      216       +4     
Flag Coverage Δ
javascript 69.75% <ø> (ø)
unittests 57.05% <57.14%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
server/portal/apps/accounts/api/urls.py 100.00% <100.00%> (ø)
server/portal/apps/accounts/urls.py 100.00% <100.00%> (ø)
server/portal/apps/auth/urls.py 100.00% <100.00%> (ø)
server/portal/apps/notifications/routing.py 0.00% <ø> (ø)
server/portal/apps/notifications/urls.py 100.00% <100.00%> (ø)
server/portal/apps/projects/views.py 34.23% <ø> (ø)
server/portal/apps/users/urls.py 100.00% <100.00%> (ø)
server/portal/apps/webhooks/models.py 81.25% <100.00%> (ø)
server/portal/apps/workspace/api/urls.py 100.00% <100.00%> (ø)
server/portal/settings/unit_test_settings.py 99.04% <ø> (ø)
... and 4 more

... and 1 file with indirect coverage changes

# Django Channels
'channels',
'daphne',

Copy link
Contributor Author

Choose a reason for hiding this comment

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


filterwarnings =
ignore::DeprecationWarning:twisted.*:
ignore::django.utils.deprecation.RemovedInDjango50Warning
markers =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still need to filter a couple of warnings.

  • The twisted package has a PR open currently to address the cgi deprecation warning
  • We can ignore any RemovedInDjango50Warning since Django 5 support is still limited at the moment

@@ -7,5 +7,5 @@
from portal.apps.notifications.consumers import NotificationsConsumer

websocket_urlpatterns = [
re_path(r'ws/notifications/$', NotificationsConsumer),
re_path(r'ws/notifications/$', NotificationsConsumer.as_asgi()),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

application = get_default_application()
application = ProtocolTypeRouter({
"http": get_asgi_application(),
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@jarosenb jarosenb Nov 6, 2023

Choose a reason for hiding this comment

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

Do websocket notifications work without defining a mapping for "websocket" in the ProtocolTypeRouter? https://channels.readthedocs.io/en/latest/topics/routing.html?highlight=protocoltyperouter#protocoltyperouter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The websocket mapping is defined in routing.py and from my testing notifications are working fine

@shayanaijaz shayanaijaz changed the title WIP: Django upgrade task/WP-271 Upgrade Django and 3rd party dependancies Nov 6, 2023
@shayanaijaz shayanaijaz marked this pull request as ready for review November 6, 2023 23:13
Copy link
Member

@rstijerina rstijerina left a comment

Choose a reason for hiding this comment

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

Nice work!

server/portal/settings/settings.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@chandra-tacc chandra-tacc left a comment

Choose a reason for hiding this comment

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

Nice work, LGTM. Tested the changes locally. I wiped my postgres data and restarted fresh.
May be there is a better way - like upgrading postgres for local users.

  • Created a job
  • checked webhook usage with job status, interactive job status.

With upgrade from 3 to 4 with django, how we handle rollback. Lets say, we deploy to dev.cep, and rollback, does that work well?

@rstijerina
Copy link
Member

With upgrade from 3 to 4 with django, how we handle rollback. Lets say, we deploy to dev.cep, and rollback, does that work well?

Rolling back would probably require restoring from a database backup

Copy link
Member

@rstijerina rstijerina left a comment

Choose a reason for hiding this comment

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

LGTM!

@chandra-tacc chandra-tacc merged commit 4fc21b6 into main Nov 21, 2023
5 of 6 checks passed
@chandra-tacc chandra-tacc deleted the task/WP-271--django-upgrade branch November 21, 2023 22:07
@chandra-tacc chandra-tacc changed the title task/WP-271 Upgrade Django and 3rd party dependancies task/WP-271 Upgrade Django to 4.2 and update other 3rd party dependencies Nov 27, 2023
chandra-tacc added a commit that referenced this pull request Dec 8, 2023
* updated django version and dependencies

* upgraded channels to version 3 and fixed tests

* upgraded channels to version 4

* formatting fix

* Added nginx conf

* add pytest filterwarning

* removed added nginx conf, added CSRF trusted origin for cep.test

* upgrade postgres to v14.9

* linting, added CSRF setting to settings_default

* update settings

---------

Co-authored-by: Sal Tijerina <[email protected]>
Co-authored-by: Chandra Y <[email protected]>
chandra-tacc added a commit that referenced this pull request Dec 8, 2023
* updated django version and dependencies

* upgraded channels to version 3 and fixed tests

* upgraded channels to version 4

* formatting fix

* Added nginx conf

* add pytest filterwarning

* removed added nginx conf, added CSRF trusted origin for cep.test

* upgrade postgres to v14.9

* linting, added CSRF setting to settings_default

* update settings

---------

Co-authored-by: Sal Tijerina <[email protected]>
Co-authored-by: Chandra Y <[email protected]>
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.

4 participants