Delete archived letter attachments from s3#4894
Conversation
Function based on a similar task for template email files
1e2c76e to
5599157
Compare
for scoped files. Also move archiving a letter attachment into a dao method - that's a more appropriate place for it.
The scoping datetimes archived_before and archived_after were defined as timezone-aware datetimes, which didn't play well with the naive datetimes from the db (for archived_at) field. After a Slack discussion, I decided to try the approach from sqlalchemy docs to try and fix it: https://docs.sqlalchemy.org/en/20/core/custom_types.html#store-timezone-aware-timestamps-as-timezone-naive-utc The scoping now works as expected.
5599157 to
d809694
Compare
string for archived_after
d7abe81 to
5f4d19a
Compare
between two tasks instead of duplicating it.
5f4d19a to
f13be34
Compare
| S3_BUCKET_INVALID_PDF = os.environ.get("S3_BUCKET_INVALID_PDF") | ||
| S3_BUCKET_TRANSIENT_UPLOADED_LETTERS = os.environ.get("S3_BUCKET_TRANSIENT_UPLOADED_LETTERS") | ||
| S3_BUCKET_LETTER_SANITISE = os.environ.get("S3_BUCKET_LETTER_SANITISE") | ||
| S3_BUCKET_LETTER_ATTACHMENTS = os.environ.get("S3_BUCKET_LETTER_ATTACHMENTS", "local-letter-attachments") |
There was a problem hiding this comment.
Can we follow the pattern of other buckets and have os.environ.get("S3_BUCKET_LETTER_ATTACHMENTS") here, and then hardcode development-letter-attachments in the Development config and test-letter-attachments in the Test config?
It keeps it consistent, plus we don't have buckets with the prefix of local-, so there isn't a bucket called local-letter-attachments
There was a problem hiding this comment.
The pattern for S3_BUCKET_TEMPLATE_EMAIL_FILES
notifications-api/app/config.py
Line 561 in 4d12732
There was a problem hiding this comment.
sure thing, I will change it 🙌🏼
| bucket_name = current_app.config["S3_BUCKET_TEMPLATE_EMAIL_FILES"] | ||
|
|
||
| @notify_celery.task(name="remove-archived-letter-attachments-from-s3") | ||
| @cronitor("remove-archived-letter-attachments-from-s3") |
There was a problem hiding this comment.
I couldn't see a corresponding credentials PR, but we will need one when this is ready to be merged (https://github.com/alphagov/notifications-manuals/wiki/Monitoring-and-alerting#cronitor)
There was a problem hiding this comment.
Ahh, good spot, I will follow the process in the manual 🙌🏼
| failed += 1 | ||
| def _get_archived_before_and_after(archived_after): | ||
| archived_before = ( | ||
| (datetime.now(UTC) - timedelta(days=ARCHIVED_FILE_RETENTION_DAYS)).astimezone(UTC).replace(tzinfo=None) |
There was a problem hiding this comment.
Since timedelta is acting on datetime.now(UTC) which is already UTC aware, isn't astimezone(UTC) redundant here?
There was a problem hiding this comment.
.ie would timedelta(days=ARCHIVED_FILE_RETENTION_DAYS)).replace(tzinfo=None) be sufficient
|
|
||
| if archived_after > archived_before: | ||
| raise ValueError( | ||
| f"Invalid archived_after for s3 cleanup of archived files: " |
There was a problem hiding this comment.
This aspect of the refactoring makes the error message less clear, it replaces "Invalid archived_after for archived template email file s3 cleanup: " and "Invalid archived_after for archived letter attachment s3 cleanup: " with less specificity.
There was a problem hiding this comment.
I thought it wouldn't matter since this error message can only be seen when running the task manually - and then that person knows which task they are running.
3 participants
I used
remove_archived_template_email_files_from_s3()task as a base for this new task.I had to adjust it, as letter attachments are a bit different. I did some refactoring on the way, and de-duplicated the code shared between the tasks.
I have pushed these changes to dev-b, and functional tests pass: https://concourse.notify.tools/teams/dev-b/pipelines/deploy-notify/jobs/functional-tests/builds/520
I also did manual testing.
I needed to make letter attachments S3 bucket available to api apps: https://github.com/alphagov/notifications-aws/pull/3045 (this needs to be merged / possibly terraform to be run first)
After that, the task worked as expected:
