Increase readtimeout value for S3 connections #1644
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #1644 +/- ##
==========================================
+ Coverage 92.89% 92.91% +0.01%
==========================================
Files 29 30 +1
Lines 4984 4995 +11
==========================================
+ Hits 4630 4641 +11
Misses 354 354 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
ebbaoliveberg
left a comment
ebbaoliveberg
left a comment
There was a problem hiding this comment.
Some comments about imports, please look at them.
| The time when it was issued is recorded to put an expiration time on the token. | ||
| """ | ||
| from dds_web.utils import current_time |
There was a problem hiding this comment.
Why import in the functions and not in the top of the file?
There was a problem hiding this comment.
Importing it at the top of the file created a circular dependency issue when utils is importing models while models was trying to import utils
| db.DateTime(), | ||
| nullable=True, | ||
| default=dds_web.utils.current_time, | ||
| default=lambda: datetime.datetime.utcnow(), |
There was a problem hiding this comment.
What is the reason for changing this?
There was a problem hiding this comment.
this one is related to the previous, in that I went to calling datetime.datetime.utcnow() instead of utils.current_time due to the circular imports issue, but then the tests failed because freezegun does not work as expected if utcnow() is called as function reference or when the Python module loads (once at startup). When it is called as lambda function, apparently SQLAlchemy calls the lambda when creating each row in the test execution.
I hope this isn't too confusing
There was a problem hiding this comment.
I mean, maybe there's better solution but I also saw the warning "The method "utcnow" in class "datetime" is deprecated" and decided that this should be addressed in a separate task
ebbaoliveberg
left a comment
ebbaoliveberg
left a comment
There was a problem hiding this comment.
No obvious issues as far as I can tell.
Pull Request Template
Before Marking as Ready for Review
dev(or other targeted branch)SPRINTLOG.mdif neededIf the target branch is
master:Summary
We have increased the readtimeout value in the CLI, this does the same change to the API since the API handles the deletion of contents in the S3 buckets.
urllib3.exceptions.ReadTimeoutError: AWSHTTPSConnectionPool(host='[s3a3.sto2.safedc.net](http://s3a3.sto2.safedc.net/)', port=443): Read timed out.Related Issue/Ticket
HMS-2533
Testing
If applicable: How did you verify the change? Include commands, data, or screenshots.
Reviewer Notes
Anything that helps reviewers (e.g. areas needing close attention).
Once all boxes are checked, mark the PR as Ready for Review and tag at least one team member as the initial reviewer.