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

GNIP 98: Django upgrade to 4.2 LTS #11821

Closed
3 of 5 tasks
mattiagiupponi opened this issue Jan 10, 2024 · 16 comments · Fixed by #11829
Closed
3 of 5 tasks

GNIP 98: Django upgrade to 4.2 LTS #11821

mattiagiupponi opened this issue Jan 10, 2024 · 16 comments · Fixed by #11829
Assignees
Labels
gnip A GeoNodeImprovementProcess Issue master
Milestone

Comments

@mattiagiupponi
Copy link
Contributor

mattiagiupponi commented Jan 10, 2024

GNIP 98 - Django upgrade to 4.2 LTS

Overview

As pointed out by the community, soon Django 3.2 will be deprecated in April 2024 in favor of 4.2

image

Proposed By

@mattiagiupponi
@cesar-benjamin

Assigned to Release

This proposal is for GeoNode 4.2/master.

State

  • Under Discussion
  • In Progress
  • Completed
  • Rejected
  • Deferred

Motivation

Django 3.2 will be no longer supported.

Proposal

This upgrade can be quite challenging due to the different geonode dependencies as also mentioned in #11722.
Many geonode dependencies are not maintained and this is blocking the upgrade.
So far the libraries that are raising issues with django 4.2 (list in progress):

  • dj-pagination
  • dynamic-rest
  • geonode-announcements
  • geonode-django-activity-stream
  • geonode-OAuth-toolkit
  • geonode-user-messages
  • pinax-ratings

For the above libraries, we should evaluate if:

  • Fork the original repo and maintain it
  • remove the functionality in geonode or find a library maintained that provides the same functionality

Other than the libraries, there are some fixes to be done:

  • The {% ifequal %} and {% ifnotequal %} template tags are removed.
  • django.conf.urls.url() is removed.

More info here https://docs.djangoproject.com/en/4.2/releases/4.0/#features-removed-in-4-0

Notes

An initial work has been done by @cesar-benjamin and is available here

Feedback

Update this section with relevant feedback, if any.

Voting

Project Steering Committee:

  • Alessio Fabiani:
  • Francesco Bartoli:
  • Giovanni Allegri:
  • Simone Dalmasso:
  • Toni Schoenbuchner: ✓
  • Florian Hoedt:

Links

Remove unused links below.

@mattiagiupponi mattiagiupponi added gnip A GeoNodeImprovementProcess Issue master labels Jan 10, 2024
@mattiagiupponi mattiagiupponi self-assigned this Jan 10, 2024
@mattiagiupponi
Copy link
Contributor Author

mattiagiupponi commented Jan 10, 2024

For who is interested, i was able to upgrade geonode by changing the minimum as possible.
A draft PR is available here #11829

In the end, the main changes are:

  • Upgrade django to 4.2.9
  • Upgrade resframework to 3.14.0
  • upgrade django-autocomplete-light to 3.9.5
  • Dynamic rest is not compatible (neither the master version) to make some test I used another fork. Ofc it will be needed to fork the main repo under GeoNode
  • pinax-ratings is not mantained anymore, i used the upgraded version done by @cesar-benjamin Ofc it will be needed to fork the main repo under GeoNode
  • pinax-notifications is not mantained anymore, i used an upgraded fork already available Ofc it will be needed to fork the main repo under GeoNode
  • geonode-mapstore-client and geonode-importer are already on Geonode, so i just update them
  • geonode-user-messages and geonode-announcements are already internal forks, so i just updated them
  • geonode-django-activity-stream was not providing anything new to the original library, so i upgrade to the original library instead: django-activity-stream==2.0.0

image

What is missing:

  • Keyword and Thesaurus select in metadata editor are not working anymore
  • Evaluate to upgrade also some other libraries
  • Complete test run

@cesarbenjamindotnet
Copy link

Thanks, I made a lot of forks to upgrade the broken deps and so on. I did saw that keywords and thesaurus will not work because some strange in autocomplete light, but regions uses autocomplete too but in another way, maybe is a good point to standardize all the forms in a clean way. And dropping old dependencies like jQuery etc. I see in mapstore client at user profile a thing that calls AngularJS... Why there is at the same time react AngularJS and jQuery? This can become be unmaintainable...

@mattiagiupponi
Copy link
Contributor Author

maybe is a good point to standardize all the forms in a clean way. And dropping old dependencies like jQuery etc. I see in mapstore client at user profile a thing that calls AngularJS... Why there is at the same time react AngularJS and jQuery? This can become be unmaintainable...

Starting from version 4.0, we moved the FE from Geonode to the mapstore client.
The wizard upgrade is tracked here #11511.

I suggest leaving this matter out of the issue/PR and instead focusing on upgrading Django to a newer version without disrupting backward compatibility.

@mattiagiupponi
Copy link
Contributor Author

sa decided with @giohappy we will remove pinax-ratings

@mattiagiupponi
Copy link
Contributor Author

as decided with @giohappy we will remove django-haystack==3.2.1 and elasticsearch

@mattiagiupponi mattiagiupponi linked a pull request Jan 12, 2024 that will close this issue
12 tasks
mattiagiupponi added a commit that referenced this issue Jan 12, 2024
mattiagiupponi added a commit that referenced this issue Jan 12, 2024
mattiagiupponi added a commit that referenced this issue Jan 12, 2024
mattiagiupponi added a commit that referenced this issue Jan 12, 2024
mattiagiupponi added a commit that referenced this issue Jan 12, 2024
mattiagiupponi added a commit that referenced this issue Jan 12, 2024
mattiagiupponi added a commit that referenced this issue Jan 12, 2024
@cesarbenjamindotnet
Copy link

as decided with @giohappy we will remove django-haystack==3.2.1 and elasticsearch

Sorry, why? i guess haystack/elasticsearch is a good complement to improve any django project about searching

@mattiagiupponi
Copy link
Contributor Author

Sorry, why? i guess haystack/elasticsearch is a good complement to improve any django project about searching

That part is unmaintained since geonode 33x. The search was changed in geonode 4.0 and that part was not mainteined anymore.

@cesarbenjamindotnet
Copy link

Hello, sorry, i was a bit afk because vacations and so on, i'd return and available again. I just noticed that there is a new release of 4.2.0, i want to continue with the integration of django 4.2, so, i have to ask you what branch i need to take, because my changes were on 4.1.x so i have to make my changes on a new branch, but i'm not sure about choosing 4.2.x or 4.2.0 or master. Please let me know what brach is the best choice to make my contrib proposals.

@mattiagiupponi
Copy link
Contributor Author

mattiagiupponi commented Jan 19, 2024

Hello, sorry, i was a bit afk because vacations and so on, i'd return and available again. I just noticed that there is a new release of 4.2.0, i want to continue with the integration of django 4.2, so, i have to ask you what branch i need to take, because my changes were on 4.1.x so i have to make my changes on a new branch, but i'm not sure about choosing 4.2.x or 4.2.0 or master. Please let me know what brach is the best choice to make my contrib proposals.

I already done the main integration to the master branch starting from your changes and are available in this branch (ISSUE_11821) in the geonode repo. The PR with the changes is this one #11829

The open points for now are:

  • Fixing the tests that are failing on CircleCI
  • Fix the select box for the keywords and the thesaurus

If you want to contribute on this points, please start from my branch and open a PR with the changes to the ISSUE_11821 branch

@cesarbenjamindotnet
Copy link

thanks, i just made a PR but some tests have failed, in fact for a strange reason i have a strange error when i use geonode-project 4.2.x branch, i can't see why but compose tries to get image from dockerhub and of course it don't find it i can't understand yet the whole geonode-project changes in 4.2.x branch, and dont work well, but with geonode-project 4.1.x and geonode 4.2.x it builds and run well. Please check my PR there is a lot of changes, not only the dependencies references described early.

@cesarbenjamindotnet cesarbenjamindotnet mentioned this issue Jan 21, 2024
10 tasks
@cesarbenjamindotnet
Copy link

@mattiagiupponi i can't send PR because CLA, what i need to sing the CLA?

@mattiagiupponi
Copy link
Contributor Author

@mattiagiupponi i can't send PR because CLA, what i need to sing the CLA?

is in the CONTRIBUTING file.

@mattiagiupponi
Copy link
Contributor Author

The PR is ready, the dependecies are updated at the minimum to keep the compatibility.
To anyone that is interested, please try it on :)

giohappy added a commit that referenced this issue Jan 31, 2024
* Upgrade to django 4.2

* Upgrade to django 4.2

* Fix tests

* black and flake fix

* Drop support of pinax-ratings

* Drop support of pinax-ratings

* black and flake fix

* black and flake fix

* remove support to HAYSTACK_SEARCH

* remove support to HAYSTACK_SEARCH

* remove pinax_ratings_tags

* Fix warning on lock acquire

* [Fixes #11821] rollback SKIP_PERMS_FILTER

* [Fixes #11821] fix dependencies

* [Fixes #11821] remove unused test of raitings

* [Fixes #11821] use geonode fork for dynamic rest

* [Fixes #11821] use geonode fork for dynamic rest

* [Fixes #11821] use geonode fork for dynamic rest

* [Fixes #11821] Fix setup.cfg

* [Fixes #11821] fix dependencies

* [Fixes #11821] fix dependencies

* [Fixes #11821] fix dependencies

* [Fixes #11821] fix dependencies

* [Fixes #11821] fix setup.cfg

* [Fixes #11821] fix setup.cfg

* [Fixes #11821] fix setup.cfg

* [Fixes #11821] fix setup.cfg

* [Fixes #11821] fix  requirements.txt

* remove default_app_config since is deprecated since django 3.2

* Define apps.py for app registration according to Django 4.2

* Define apps.py for app registration according to Django 4.2

* [Fixes #11821] Fix tests

* [Fixes #11821] Add apps.py and fix test import

* [Fixes #11821] Add apps.py and fix test import

* [Fixes #11821] Add apps.py and fix test import

* merge with master

* upgrade requirement

* Update setup.cfg

* Update setup.cfg

* Fix select autocomplete light

---------

Co-authored-by: Giovanni Allegri <[email protected]>
@giohappy giohappy added this to the 4.4.0 milestone Jan 31, 2024
@cesarbenjamindotnet
Copy link

Hello, thanks for the updates.

I'm trying run this from geonode-project but there is a problem, all goes fine except running the django container, all others containers run well but django4geonode thows Fail, if i inspect the logs it seems all tasks are complete and i can see uwsgi runs but container keeps restarting

@cesarbenjamindotnet
Copy link

i see there is differences about mapstore client between requirements.txt and setup.cfg, btw

@cesarbenjamindotnet
Copy link

as decided with @giohappy we will remove django-haystack==3.2.1 and elasticsearch

in tasks.py this code allways go to exception:

def migrations(ctx):
    print("**************************migrations*******************************")
    ctx.run(f"python manage.py migrate --noinput --settings={_localsettings()}", pty=True)
    ctx.run(
        f"python manage.py migrate --noinput --settings={_localsettings()} --database=datastore",
        pty=True,
    )
    try:
        ctx.run(
            f"python manage.py rebuild_index --noinput --settings={_localsettings()}",
            pty=True,
        )
    except Exception:
        pass

because python manage.py rebuild_index is a haystack command, so is not needed that last try-catch block, i guess can be removed, i did open #11920 for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gnip A GeoNodeImprovementProcess Issue master
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants