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

CLE-3128 django42 upgrade fixes #56

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

DelboyJay
Copy link
Contributor

@DelboyJay DelboyJay commented Aug 30, 2024

Changes that are required for projects that consume this one to run Django 4.2.

Added

  • Support for Python 3.12
  • This changelog to track future changes!
  • Pre-commit checks and run them as psrt of the Github actions workflow.
  • Pre-commit checks for missing migrations
  • Make file contains command db-setup to test running all migrations on an empty database

Removed

  • Support for Django 2.2 and python 3.7 & 3.8

@DelboyJay DelboyJay force-pushed the CLE-3128_django42_upgrade_fixes branch from a402c7a to 50a97f3 Compare August 30, 2024 09:52
Del Hyman-Jones added 2 commits August 30, 2024 11:25
Added `make dbrun` to run postgres container
Generated missing migrations and ran `make db-setup` to prove migrations still run on a fresh database.
Added pre-commit config based off of Platform and ran it over all files.
@DelboyJay DelboyJay force-pushed the CLE-3128_django42_upgrade_fixes branch from 97e467d to 3f4340e Compare August 30, 2024 10:29
Del Hyman-Jones added 2 commits August 30, 2024 11:31
…running pytest unittests

* Added some settings based on recommendations from `poetry run ./manage.py check --deploy`
* Renamed classes that start with the word `Test` but are not pytest classes as this was confusing pytest.
* A new migration is required after the package update. `make db-setup` was run to prove the migrations run on an empty database.
* Use python 3.12 for running github workflow lint checks now that our environment is working for it.
@DelboyJay DelboyJay force-pushed the CLE-3128_django42_upgrade_fixes branch from 9da73a9 to 326d92c Compare August 30, 2024 11:27
@@ -7,10 +7,10 @@ runs:
using: "composite"
steps:
- name: Check out repository
uses: actions/checkout@v3
uses: actions/checkout@v4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixes a warning from github about the actions being forced to use node.js 20.

@@ -19,7 +19,7 @@
# Quick-start development settings - unsuitable for production
# See https://docs.djangoproject.com/en/3.0/howto/deployment/checklist/

SECRET_KEY = "mysecret"
SECRET_KEY = "231cc7b8-63ce-4a87-98ea-4c663ee9557d:d7bcde56-e812-4ef0-aa07-3b22f3066d2f"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just for an example app so does not matter but fixed a django check warning I was seeing.

TestInternalWithFormIntegration,
TestLocalIntegration,
TestLocalWithFormIntegration,
InternalIntegrationTest,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

having test at the start of the name confuses pytest as these are not true pytest classes

- Tell black that we are targeting python 3.9 to 3.12
- added a MAKING_RELEASES.md file.
- added bump2version to auto update the code with the new version number moving forward.
- added twine to push the new release to pypi.
- added more make file command around version bumping and , package building and releasing
Del Hyman-Jones added 2 commits August 30, 2024 16:48
- added a MAKING_RELEASES.md file.
- added bump2version to auto update the code with the new version number moving forward.
- added twine to push the new release to pypi.
- added more make file command around version bumping and , package building and releasing
Comment on lines +12 to +16
# Django does not explicitly follow the dependency definitions under the
# `dependencies` variables as you might expect.
# It is imperative that these migrations are run BEFORE
# oauth2_provider.0004_auto_20200902_2022, or we get errors because the
# drf_integrations.Application model cannot be found

Choose a reason for hiding this comment

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

this should be documented as part of the setup instructions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a section under README.md to mention this when updating the migrations in case we come across it in the future.

Copy link

@lewis-kori lewis-kori left a comment

Choose a reason for hiding this comment

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

Looks good. I think we should add a CONTRIBUTING.rst as a guideline for how open source contributors can get involved with the project.

example: https://github.com/saltpay/drf-jwt-auth/blob/master/CONTRIBUTING.rst

CHANGELOG.md Outdated

Choose a reason for hiding this comment

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

Can we rely on GitHub releases to generate the changelogs? One can develop that there as you make a release. It'd be an ideal setup as one can see the change log through the GitHub interface as you track the packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically we should do both. CHANGELOG.md is a dev technical level and github releases is for user-based top-level overview.

README.md Show resolved Hide resolved
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.

None yet

3 participants