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.

Sign up for GitHub

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

Jump to bottom

Django upgrade to 4.2 #2031

Closed
wants to merge 3 commits into from
Closed

Django upgrade to 4.2 #2031

wants to merge 3 commits into from

Conversation

szabozoltan69
Copy link
Contributor

Refers to #1990

@szabozoltan69 szabozoltan69 changed the title W.i.p Django upgrade to 4.2 Feb 6, 2024
@szabozoltan69 szabozoltan69 force-pushed the feature/upgrade-to-django-4-2 branch 13 times, most recently from fa42c6c to 1678dae Compare February 11, 2024 21:34
@szabozoltan69 szabozoltan69 marked this pull request as ready for review February 12, 2024 08:44
thenav56
thenav56 requested changes Feb 13, 2024
View reviewed changes
Copy link
Member

@thenav56 thenav56 left a comment

Choose a reason for hiding this comment

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

Thanks for the upgrade :D
Few comments/questions

docker-compose.yml
@@ -61,7 +61,7 @@ x-server: &base_server_setup

services:
db:
image: postgis/postgis:11-3.1-alpine
image: postgis/postgis:12-3.4-alpine
Copy link
Member

Choose a reason for hiding this comment

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

Hey @szabozoltan69, doing this might break current setup. Mabye we should use different directory instead for the data? or use a volume instead?
We will also need to specify how to move data from old db version to new one.

./.db-12/pg:/var/lib/postgresql/data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea! (Maybe I'll use .db/pg-12)
The new db can be done via a dump and a load, like this:

pg_dump -O    -c --if-exists -h 127.0.0.1 -U test test > /tmp/testdump.sql

Then the load is via a simple \i ...testdump.sql from the new db.

Copy link
Member

Choose a reason for hiding this comment

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

Nice... let's add this to PR description as well

Dockerfile Show resolved Hide resolved
api/serializers.py Outdated
@@ -2061,7 +2061,7 @@ class SearchEmergencySerializer(serializers.Serializer):
class SearchSurgeAlertSerializer(serializers.Serializer):
id = serializers.IntegerField()
name = serializers.CharField()
keywords = serializers.ListField(serializers.CharField())
keywords = serializers.ListField()
Copy link
Member

Choose a reason for hiding this comment

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

Hey @szabozoltan69, Any reason why we are removing child definition here?
Maybe we need to use child=serializers.CharField() instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Placed back (and the other places also) as

...  = serializers.ListField(child=serializers.CharField())

@szabozoltan69
Copy link
Contributor Author

Thanks for the great catches, @thenav56 , all of them are relevant and useful, I'll fix them soon!

@szabozoltan69 szabozoltan69 force-pushed the feature/upgrade-to-django-4-2 branch from ec97be0 to 9689f73 Compare February 13, 2024 14:55
@szabozoltan69 szabozoltan69 force-pushed the feature/upgrade-to-django-4-2 branch from 9689f73 to de96934 Compare February 13, 2024 17:31
@szabozoltan69 szabozoltan69 force-pushed the feature/upgrade-to-django-4-2 branch 2 times, most recently from a1c0aee to bceed17 Compare February 28, 2024 14:53
szabozoltan69
szabozoltan69 commented Mar 6, 2024
View reviewed changes
api/admin.py
admin.site.unregister(Token)
admin.site.register(Token, GoTokenAdmin)
# admin.site.unregister(Token)
# admin.site.register(Token, GoTokenAdmin)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it really removable? So strange...

@szabozoltan69
Copy link
Contributor Author

Also python-magic = "==0.4.27" to packages.

@k9845 k9845 force-pushed the feature/upgrade-to-django-4-2 branch from af36a63 to 490d638 Compare May 2, 2024 08:09
@szabozoltan69
Copy link
Contributor Author

szabozoltan69 commented May 2, 2024 •
edited
Loading

Hi @k9845 , I think we should let this branch go and initiate another branch for Django 4.2 cutover. It would be hard to keep this one fine and usable (and not overwriting anyting).

@k9845 k9845 force-pushed the feature/upgrade-to-django-4-2 branch 2 times, most recently from 7dca97b to 618bf42 Compare May 2, 2024 08:18
@k9845 k9845 force-pushed the feature/upgrade-to-django-4-2 branch from 618bf42 to 7806d07 Compare May 14, 2024 08:36
@k9845 k9845 force-pushed the feature/upgrade-to-django-4-2 branch from 7806d07 to 8957d47 Compare May 14, 2024 08:37
@szabozoltan69
Copy link
Contributor Author

Will be closed.
Reimplemented via #2152

@szabozoltan69 szabozoltan69 deleted the feature/upgrade-to-django-4-2 branch May 22, 2024 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thenav56 thenav56 thenav56 approved these changes

@batpad batpad Awaiting requested review from batpad

@k9845 k9845 Awaiting requested review from k9845

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@szabozoltan69 @thenav56