-
Notifications
You must be signed in to change notification settings - Fork 48
Update Fence to Python 3.13 + Run as gen3 user #1312
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
Conversation
…ocal_settings.py`
…dependency_updates
…dependency_updates
…update_fence_3_13
Pull Request Test Coverage Report for Build 20471262045Details
💛 - Coveralls |
|
Failed to Prepare CI environment Please find the Github Action logs here |
Please find the detailed integration test report here Please find the Github Action logs here |
Please find the detailed integration test report here Please find the Github Action logs here |
tests/test_app_config.py
Outdated
| patchers.append(patcher) | ||
|
|
||
| # create a fresh local app | ||
| from flask import Flask |
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.
This did not throw an error in older versions of fence due to the order in which tests ran.
| headers = {"Authorization": "Bearer " + encoded_creds_jwt.jwt} | ||
| response = client.delete("/data/{}".format(did), headers=headers) | ||
| assert response.status_code == 204 | ||
| assert mock_boto_delete.called_once() |
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.
Since IndexedFile.delete_files is patched with a MagicMock, we never really hit the core boto delete functionality in the past, since we were patching IndexedFile.delete_files before we reached the core functionality we wanted to test
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 tests were passing in the past, because assert .called_once() returned a new MagicMock which is non-falsy in nature, making the assert to pass
| """ | ||
| raise Exception("url not available") | ||
|
|
||
| with patch("fence.blueprints.data.indexd.flask.current_app", return_value=app): |
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.
Since flask is coming from a fixture, flask.current_app is already set to app.
Setting this patch, was making everything internally to MagicMock. Removing this patch altogether tests the logic better
| assert username_to_log_in == username | ||
| iss_sub_pair_to_user_records = db_session.query(IssSubPairToUser).all() | ||
| assert len(iss_sub_pair_to_user_records) == 1 | ||
| iss_sub_pair_to_user = db_session.query(IssSubPairToUser).get((iss, sub)) |
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.
# LegacyAPIWarning: The Query.get() method is considered legacy as of the 1.x series of SQLAlchemy and becomes a legacy construct in 2.0. The method is now available as Session.get() (deprecated since: 2.0) (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
|
Failed to Prepare CI environment Please find the Github Action logs here |
Please find the detailed integration test report here Please find the Github Action logs here |
paulineribeyre
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.
just minor comments!
pyproject.toml
Outdated
| "gunicorn", # not imported but used in Dockerfile | ||
| "psycopg2-binary", # not imported but used by sqlalchemy | ||
| "urllib3", # Needed to be pinned to a version >=2.6.0 | ||
| "sniffio", # required by gen3authz, but not installed as a transitive dependency during poetry install |
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 added it here, if you upgrade gen3authz to the latest version you should be able to remove sniffio 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.
gen3authz 3.0.0 requires backoff>=2.2.1, but fence has backoff pinned to <2 several years ago. I'm trying to remove that restriction and run unit tests to see if anything breaks.
Please find the detailed integration test report here Please find the Github Action logs here |
| run_async(updater_queue.put(mock_users[0])) | ||
|
|
||
| # Mock the client to return a valid update process | ||
| mock_oidc_clients[0].update_user_authorization = AsyncMock() |
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 method update_user_authorization (https://github.com/uc-cdis/fence/blob/e288114ca81d31c2abd28dbd20d2d64e18ed87e0/fence/resources/openid/idp_oauth2.py#L446C20-L446C20) is not Async to use AsyncMock
paulineribeyre
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.
Looks good, just need to update gen3authz and remove the sniffio dependency if you can
Please find the detailed integration test report here Please find the Github Action logs here |
Please find the detailed integration test report here Please find the Github Action logs here |
Link to JIRA ticket if there is one: PD-77
New Features
Breaking Changes
Bug Fixes
Improvements
session.execute()withsqlalchemy.textdatetime.utcnow()anddatetime.utcfromtimestamp()with 3.13 compatible equivalentsrequestattributes withrequest.payloadquery.get()withSession.get()botousage in favor ofboto3strtoboolsincedistutilsis no longer available since Python 3.12NoAsyncMagicMocksinceMagicMockreturnsAsyncMockseven if the source object is partially asyncassert <mocked_method>.not_calledwith<mocked_method>.assert_not_calledDependency updates
Deployment changes
gen3user instead ofroot