PD-90 PD-92 Python to 3.13 and replace Gino with sqlalchemy#82
Conversation
|
The style in this PR agrees with This formatting comment was generated automatically by a script in uc-cdis/wool. |
| CURRENT_DIR = os.path.dirname(os.path.realpath(__file__)) | ||
|
|
||
|
|
||
| def test_all_migrations_have_tests(): |
There was a problem hiding this comment.
copied from gen3-workflow
| def access_token_patcher(client, request, access_token_patcher_param): | ||
| """ | ||
| Patch the `access_token` function. By default, this fixture is parametrized to return |
There was a problem hiding this comment.
fixed the access token fixtures - the tests were previously running twice even when using access_token_user_only_patcher because of params ["user_token", "client_token"] in autoused access_token_user_client_patcher
|
Failed to Prepare CI environment Please find the Github Action logs here |
| ) | ||
| ).one() | ||
| except IntegrityError as e: | ||
| if "asyncpg.exceptions.UniqueViolationError" in str(e): |
There was a problem hiding this comment.
This raised a question for me around the handling of request_id. My understanding is that the request_id is generated server-side as a UUID (here), not provided by the user.
In the unlikely event that a duplicate does occur, should we be returning a 4xx error to the user? From the user’s perspective, the request itself is valid, and the collision is a system-generated artifact rather than an issue with client input.
If that is the case, we might want to try a retry mechanism with a new request_id in such case.
Let me know if I misunderstood this somehow.
There was a problem hiding this comment.
Yes a retry would be better. I would rather merge the PR asap and not make further changes but I could add a TODO
In any case, like you said this is very unlikely to happen
| def load_modules(app: FastAPI = None) -> None: | ||
| # FIXME: Identify the cause for duplicate entry points (PXP-8443) | ||
| # Added a set on entry points to dodge the intermittent duplicate modules issue | ||
| for ep in set(entry_points()["requestor.modules"]): |
There was a problem hiding this comment.
Haven't gotten back to this issue in years. Did you investigate the issue during the course of this PR?
There was a problem hiding this comment.
I couldn't reproduce it anymore so i took my chances and removed it!
| "An error occured while updating role '{}': '{}'".format( | ||
| {role["id"]}, str(e) | ||
| ) |
There was a problem hiding this comment.
Why not change it to an f-string completely?
nss10
left a comment
nss10
left a comment
There was a problem hiding this comment.
Reviewed tests as well. Tests look good overall.
| raise | ||
|
|
||
|
|
||
| class MigrationRunner: |
| with mock.patch("requestor.request_utils.requests") as mock_requests: | ||
| # update the request status | ||
| res = client.put(f"/request/{request_id}", json={"status": "APPROVED"}) | ||
| assert res.status_code == 200, res.text |
There was a problem hiding this comment.
Tests Fail early now. Good 👍
There was a problem hiding this comment.
it makes more sense to check the status code before running assert_called_once_with and getting a weird error when the request failed
2d436cc to
6f910ac
Compare
2 participants
Link to JIRA ticket if there is one: https://ctds-planx.atlassian.net/browse/PD-90 and https://ctds-planx.atlassian.net/browse/PD-92
New Features
Breaking Changes
Bug Fixes
Improvements
Dependency updates
Deployment changes