-
Notifications
You must be signed in to change notification settings - Fork 7
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
[PP-1947] convert attachments to s3 links #2209
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2209 +/- ##
=======================================
Coverage 91.11% 91.11%
=======================================
Files 363 363
Lines 41268 41274 +6
Branches 8837 8837
=======================================
+ Hits 37600 37606 +6
Misses 2406 2406
Partials 1262 1262 ☔ View full report in Codecov by Sentry. |
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. I added a couple minor comments but feel free to merge as is if you don't want to address them.
|
||
def update_bucket_expiration_rule( | ||
self, prefix: str, expiration_in_days: int = 30 | ||
) -> None: |
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.
Minor: Seems like this might be something we want to just do in our terraform config, instead of doing it every time we do an inventory report. We already apply a lifecycle rule to expire multipart uploads.
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.
👍
prefix=f"{reports_path}/", expiration_in_days=expiration_in_days | ||
) | ||
|
||
with zip_path.open( |
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.
Minor: Instead of opening the file again here, I'm fairly sure that report_zip
is a IO[bytes]
, which store_stream
can take directly.
c7313c9
to
fe5dc35
Compare
…move bucket lifecycle configuration to terraform.
fe5dc35
to
f452d37
Compare
Description
This update changes the way we are delivering inventory and holds reports. Instead of attaching the zipped reports to an email we are instead providing a link in the email to the zip which lives in public s3 bucket. The zip file will be removed automatically after 30 days.
The url contains a UUID in order ensure that the email link is not guessable and to ensure that it is uniquely associated with the email request. In addition the zip file name includes the library short name as well as the date range covered by the request.
Motivation and Context
https://ebce-lyrasis.atlassian.net/browse/PP-1947
How Has This Been Tested?
Unit tests cover it but it has not yet been tested in dev.
Checklist