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

Stop supporting Django's older than 3.2 #2823

Merged
merged 4 commits into from
Feb 26, 2024
Merged

Conversation

hmpf
Copy link
Contributor

@hmpf hmpf commented Feb 22, 2024

No description provided.

@hmpf hmpf added the dependencies Pull requests that update a dependency file label Feb 22, 2024
Copy link

codecov bot commented Feb 22, 2024

Codecov Report

Attention: Patch coverage is 93.75000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 57.07%. Comparing base (d96a876) to head (0ca12b9).
Report is 3 commits behind head on master.

Files Patch % Lines
python/nav/web/api/v1/views.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2823      +/-   ##
==========================================
+ Coverage   57.06%   57.07%   +0.01%     
==========================================
  Files         567      567              
  Lines       41289    41255      -34     
==========================================
- Hits        23561    23548      -13     
+ Misses      17728    17707      -21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented Feb 22, 2024

Test results

     12 files       12 suites   12m 7s ⏱️
3 303 tests 3 303 ✔️ 0 💤 0
9 384 runs  9 384 ✔️ 0 💤 0

Results for commit 0ca12b9.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@johannaengland johannaengland left a comment

Choose a reason for hiding this comment

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

Can we also remove requirements/django22.txt or should that happen in a later PR?

@hmpf hmpf changed the title Stop supporting Django 2.2 Stop supporting Django's older than 2.2 Feb 23, 2024
@hmpf hmpf changed the title Stop supporting Django's older than 2.2 Stop supporting Django's older than 3.2 Feb 23, 2024
@hmpf
Copy link
Contributor Author

hmpf commented Feb 23, 2024

Can we also remove requirements/django22.txt or should that happen in a later PR?

I sincerely hope so. I cannot find anything in nav-code directly using requirements/django22.txt but what if there is some other repo (docker, or the debian package building one) that still uses it? We'll just have to wait for @lunkwill42

@lunkwill42
Copy link
Member

lunkwill42 commented Feb 26, 2024

Can we also remove requirements/django22.txt or should that happen in a later PR?

I sincerely hope so. I cannot find anything in nav-code directly using requirements/django22.txt but what if there is some other repo (docker, or the debian package building one) that still uses it? We'll just have to wait for @lunkwill42

Nah, get rid of it in this PR, please. The multiples of requirements/django*.txt files are only used to select the correct set of Django dependencies based on the current tox environment (see how tox.ini selects requirements files). The actual production requirements are always selected by the top-level /requirements.txt.

And - as you can see from tox.ini, it doesn't support environments with anything other than Django 3.2 and Django 4.0.

@hmpf hmpf self-assigned this Feb 26, 2024
@johannaengland
Copy link
Contributor

I added my own commit after playing a bit with regexes and looking through all mentions of Django in the codebase.
@hmpf you might want to have a look at the file nav/django/utils.py to see if there is any more code that can be removed/changed.

@hmpf
Copy link
Contributor Author

hmpf commented Feb 26, 2024

The multiples of requirements/django*.txt files are only used to select the correct set of Django dependencies based on the current tox environment (see how tox.ini selects requirements files).

Maybe for tox we can get rid of the requirements/django* altogether by setting the django-versions directly in the tox.ini. Then pyproject.toml could depend on only "django=max". Depends what pip-compile does.

@hmpf
Copy link
Contributor Author

hmpf commented Feb 26, 2024

I added my own commit after playing a bit with regexes and looking through all mentions of Django in the codebase. @hmpf you might want to have a look at the file nav/django/utils.py to see if there is any more code that can be removed/changed.

get_model_and_name, get_all_related_objects and get_all_related_many_to_many_objects are only used by seeddb so I'd rather move that to its own issue as (hopefully) an easy win for the next time somebody has to wrangle with seeddb.

@hmpf hmpf merged commit 8ea18f5 into Uninett:master Feb 26, 2024
7 of 8 checks passed
@hmpf hmpf deleted the drop-django-22 branch February 26, 2024 13:57
Copy link

sonarcloud bot commented Feb 26, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
D Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

idea Catch issues before they fail your Quality Gate with our IDE extension SonarLint SonarLint

@lunkwill42
Copy link
Member

The multiples of requirements/django*.txt files are only used to select the correct set of Django dependencies based on the current tox environment (see how tox.ini selects requirements files).

Maybe for tox we can get rid of the requirements/django* altogether by setting the django-versions directly in the tox.ini. Then pyproject.toml could depend on only "django=max". Depends what pip-compile does.

Maybe, maybe not. The requirements/django*.txt set of files don't always have just one line for Django. To test on specific Django versions, we sometimes also have to select alternative versions of other Django-related libraries we depend on, to ensure those libraries are installed in versions that support the targeted Django version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants