-
Notifications
You must be signed in to change notification settings - Fork 84
Python 3.13 🚂 #7096
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
Python 3.13 🚂 #7096
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
8ef216f to
39d0b5e
Compare
docs/fides/Dockerfile
Outdated
| @@ -1,10 +1,11 @@ | |||
| FROM python:3.10.16-slim-bookworm AS build | |||
| FROM python:3.12-slim-bookworm AS build | |||
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.
shouldn't this be using python 3.13?
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.
It's for generating the docs but yes, it should probably just be the same 😄
| from enum import StrEnum | ||
|
|
||
| class ManualTaskExecutionTiming(StrEnum): | ||
| """Enum for when a manual task should be executed in the privacy request DAG.""" | ||
|
|
||
| pre_execution = "pre_execution" # Execute before the main DAG | ||
| post_execution = "post_execution" # Execute after the main DAG | ||
| parallel = "parallel" # Execute in parallel with the main DAG | ||
|
|
||
|
|
||
| class ManualTaskType(str, Enum): | ||
| from enum import StrEnum |
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.
the robot is right here
| If an identity field with the same field_name already exists for this privacy request, | ||
| it will be replaced with the new value to prevent duplicate records. |
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 change needed ? 🤔
|
|
||
|
|
||
| class ConsentMechanism(str, Enum): | ||
| from enum import StrEnum |
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 we put the imports top-level ?
| ConnectionNotFoundException, | ||
| KeyOrNameAlreadyExists, | ||
| SaaSConfigNotFoundException, | ||
| ValidationError, |
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.
is this import used?
| @pytest.mark.skip("Fails in 3.13 and can probably be removed anyway.") | ||
| def test_abstract_class_attributes(self): |
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.
tagging @JadeCara since I think she knows this code best
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.
Yeah this one can be removed :)
| @pytest.mark.skip( | ||
| "This test just verifies that the S3 client can download large files" | ||
| ) |
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 are we skipping it though?
noxfile.py
Outdated
|
|
||
| REQUIRED_DOCKER_VERSION = "20.10.17" | ||
| REQUIRED_PYTHON_VERSIONS = ["3.9", "3.10"] | ||
| REQUIRED_PYTHON_VERSIONS = ["3.9", "3.10", "3.12", "3.13"] |
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 think we should just leave 3.13 here no? or at least definitely remove 3.9 and 3.10
…pollute the 3.13 functional changes with style / linter changes
erosselli
left a comment
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.
🚢
fides
|
||||||||||||||||||||||||||||||||||
| Project |
fides
|
| Branch Review |
main
|
| Run status |
|
| Run duration | 01m 35s |
| Commit |
|
| Committer | John Ewart |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
1
|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
4
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
cypress/e2e/smoke_test.cy.ts • 1 failed test
| Test | Artifacts | |
|---|---|---|
| Smoke test > can submit an access request from the Privacy Center |
Screenshots
Video
|
|
Do not merge this yet! 😆
(This PR is being opened to run all the tests)
Ticket ENG-36
Description Of Changes
Update to modern Python 🥳 🍰 🎉 🙌
Code Changes
Misc. fixes for compatibility
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works