-
Notifications
You must be signed in to change notification settings - Fork 48
Update dependencies in fence after running deptry #1302
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
Pull Request Test Coverage Report for Build 19682826605Details
💛 - Coveralls |
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 |
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.
If we want to be complete about this, take a look at the old cloud auto k8s configmaps and secrets that were relying on this: https://github.com/uc-cdis/cloud-automation/blob/master/kube/services/fence/fence-deploy.yaml#L69
And remove them from Helm: https://github.com/uc-cdis/gen3-helm/blob/ae911bca4a44c673d44f92f3827957a9338e79b9/helm/fence/values.yaml#L438
And make sure nothing breaks
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.
And update the Breaking Changes bullet point for this PR so its clear that local_settings is no longer supported.
…ocal_settings.py`
| @@ -1,104 +0,0 @@ | |||
| """ | |||
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.
Deleting this test seems appropriate, since the main purpose of these tests was to ensure that Fence doesn’t crash when local_settings.py is missing. This made sense when local_settings.py was optional after being deprecated, but it now seems unnecessary. Thoughts?
…dependency_updates
Please find the detailed integration test report here Please find the Github Action logs here |
…dependency_updates
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 |
|
|
||
| # Django stuff: | ||
| *.log | ||
| local_settings.py |
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.
let's still keep this in the .gitignore just in case some one has one, we don't want to check it in
pyproject.toml
Outdated
| # NOTE: for testing with updated libaries as git repos: | ||
| # foobar = {git = "https://github.com/uc-cdis/some-repo", rev = "feat/test"} | ||
| jinja2 = "^3.1.6" | ||
| wtforms = "^3.2.1" |
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.
if possible, change all these ^ to >=
|
Failed to Prepare CI environment 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.
lgtm just need to finish changing ^ to >=
|
Failed to Prepare CI environment Please find the Github Action logs here |
fence/settings.py
Outdated
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 remove this whole file safely?
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.
Please help me understand this. With local_settings being gone, does fence still need the ability to search for the cofig in paths described by CONFIG_SEARCH_FOLDERS
Am I to reposition this variable such that we can get rid of this file, or am I to see if it is safe to eliminate this variable altogether?
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.
reposition this variable such that we can get rid of this file
This is what I meant, sorry for being unclear. This was placed here as a bridge between the old and new config at the time (so if you had old config, it could find the new config). But now, we should just place that variable somewhere else and remove this file altogether.
We still need to search for the new config, but it can be put somewhere else. Maybe directly in the config.py:
Lines 12 to 14 in 4173550
| DEFAULT_CFG_PATH = os.path.join( | |
| os.path.dirname(os.path.abspath(__file__)), "config-default.yaml" | |
| ) |
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.
@nss10 it looks like the file is still here even though you removed the config. Can we just delete the entire file?
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.
Sorry, I missed deleting the actual file. Thanks @paulineribeyre for taking care of this :)
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 |
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
local_settings.pyis removed completely.Bug Fixes
Improvements
Dependency updates
Deployment changes