-
Notifications
You must be signed in to change notification settings - Fork 6
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
Backend version updates #605
Conversation
|
GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
---|---|---|---|---|---|
9335321 | Triggered | Generic Private Key | 74d67e1 | nginx/certs/_wildcard.localhost.net-key.pem | View secret |
9335321 | Triggered | Generic Private Key | f23ff2d | nginx/certs/_wildcard.localhost.net-key.pem | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Our GitHub checks need improvements? Share your feedbacks!
backend/Pipfile
Outdated
@@ -4,20 +4,21 @@ url = "https://pypi.org/simple" | |||
verify_ssl = true | |||
|
|||
[dev-packages] | |||
black = "==19.10b0" | |||
black = "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this still here? And why is it = "*"? (Same for flake8)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be mistaken, but I think the CI expects black and flake8 to be installed as part of the Django check, so until we migrate shared-actions to use ruff we should keep these here? I removed the version pins as I wasn't sure why they were necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just updated shared-actions to use ruff, and got rid of black, flake8, and isort
@@ -44,7 +45,7 @@ uvloop = {version = "*", sys_platform = "== 'linux'"} | |||
uvicorn = {extras = ["standard"], version = "*"} | |||
gunicorn = "*" | |||
httptools = "*" | |||
ics = "==0.7" | |||
ics = "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you just confirm we're not depending on a version pin here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was only one apparent issue with imports relating to the version pin which I fixed here
backend/Pipfile
Outdated
@@ -34,7 +35,7 @@ qrcode = "*" | |||
python-dateutil = "*" | |||
psycopg2 = "*" | |||
django-simple-history = "*" | |||
channels = "<3" | |||
channels = "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you just confirm we're not depending on a version pin here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this was the only part of our code that needed to be updated, but I'll do further checks/tests locally to make sure unpinning the version doesnt cause any issues
backend/Pipfile
Outdated
@@ -57,6 +58,8 @@ pandas = "*" | |||
drf-excel = "*" | |||
numpy = "*" | |||
coverage = "*" | |||
daphne = "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why we need daphne
and tatsu
, and why tatsu
needs a pin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
daphne
is needed because channels.testing
now requires it but doesn't install it as a subdependency (see this)
I think we should be able to get rid of the tatsu
pin. I don't remember why I added it (could've been because they dropped support for Python 3.10, but this isn't relevant as we're using 3.11 anyway).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember why I added the tatsu
pin now, it's because the latest Django base image uses Python 3.10. Will try and use an image with Python 3.11
@@ -12,6 +12,7 @@ jobs: | |||
with: | |||
projectName: pennclubs | |||
path: backend | |||
pythonVersion: 3.11-bookworm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this bookworm
stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bookworm
is the latest stable Debian release and it seems that Docker has removed support for buster
(which is two versions behind bookworm
) with later versions of Python. Also if we don't specify the image it defaults to python3.8-buster
which doesn't make sense now that we're using 3.11
Overall this looks good to me! Can you just clarify some of my comments, and tell me about all the local tests you've run with this? We should also temporarily add in integration tests to just test out a CI run with them enabled. Very possible it could catch some unwanted behaviour or runtime exceptions due to deprecated functions that the tests don't otherwise catch. A couple other things:
Here is what ChatGPT tells me to look out for (some of it is cruft but thought I'd paste here for you to read): Django has undergone several changes from version 3.2 to 5.0, including new features and some backwards incompatible changes. Here's a summary of the key backwards incompatible changes in each major version from Django 3.2 to 5.0: Django 3.2
Django 4.0
Django 4.2
Django 5.0
Each of these versions introduced specific changes that might require adjustments in your Django projects, especially if you are directly upgrading from Django 3.2 to Django 5.0. It's important to review the detailed release notes and migration guides for each version to ensure a smooth upgrade process. For detailed information, you can refer to the release notes for Django 5.0, Django 4.2, Django 4.0, and Django 3.2. |
For local testing I've only run the unit tests and backend and verified that there were no errors, but I agree that integration tests will definitely help us catch errors that the unit tests won't. I also looked at the Django deprecation page when making the upgrades and didn't notice any glaring incompatible changes, but will take a closer look to make sure I didn't miss anything |
Looks like integration tests are passing! |
LGTM. A few guidelines around the merge
|
This PR upgrades Django from version 3.2 to 5.0 (should be able to try out
GeneratedField
in #601!) and sets the Python version to 3.11 from 3 (tried upgrading to 3.12 but unfortunately some packages still need to add support for it, so it's better to wait) along with some other related upgrades.Additionally, it replaces Black, Flake8, and Isort with Ruff, which is a very fast linter and formatter. The translation from previous linting rules to Ruff's are close to but not exactly one-to-one, so we'll likely need to make some tweaks to get the configuration that we want.