Merge pull request #1330 from GSA/fix_test_coverage

actually start deleting old s3 objects

.ds.baseline

@@ -209,7 +209,7 @@
         "filename": "tests/app/aws/test_s3.py",
         "hashed_secret": "67a74306b06d0c01624fe0d0249a570f4d093747",
         "is_verified": false,
-        "line_number": 27,
+        "line_number": 28,
         "is_secret": false
       }
     ],
@@ -384,5 +384,5 @@
       }
     ]
   },
-  "generated_at": "2024-09-10T18:12:39Z"
+  "generated_at": "2024-09-11T14:31:46Z"
 }

app/aws/s3.py

@@ -87,7 +87,6 @@ def get_bucket_name():
 
 
 def cleanup_old_s3_objects():
-
     bucket_name = get_bucket_name()
 
     s3_client = get_s3_client()
@@ -99,9 +98,15 @@ def cleanup_old_s3_objects():
         while True:
             for obj in response.get("Contents", []):
                 if obj["LastModified"] <= time_limit:
+
+                    try:
+                        remove_csv_object(obj["Key"])
                         current_app.logger.info(
-                        f"#delete-old-s3-objects Wanting to delete: {obj['LastModified']} {obj['Key']}"
+                            f"#delete-old-s3-objects Deleted: {obj['LastModified']} {obj['Key']}"
                         )
+                    except botocore.exceptions.ClientError:
+                        current_app.logger.exception(f"Couldn't delete {obj['Key']}")
+
             if "NextContinuationToken" in response:
                 response = s3_client.list_objects_v2(
                     Bucket=bucket_name,

app/config.py

@@ -251,7 +251,7 @@ class Config(object):
             },
             "delete_old_s3_objects": {
                 "task": "delete-old-s3-objects",
-                "schedule": crontab(minute="*/5"),
+                "schedule": crontab(hour=7, minute=10),
                 "options": {"queue": QueueNames.PERIODIC},
             },
             "regenerate-job-cache": {

tests/app/aws/test_s3.py

@@ -1,4 +1,5 @@
 import os
+from datetime import timedelta
 from os import getenv
 
 import pytest
@@ -31,15 +32,29 @@ def single_s3_object_stub(key="foo", last_modified=None):
 
 
 def test_cleanup_old_s3_objects(mocker):
+    """
+    Currently we are going to delete s3 objects if they are more than 14 days old,
+    because we want to delete all jobs older than 7 days, and jobs can be scheduled
+    three days in advance, and on top of that we want to leave a little cushion for
+    the time being.  This test shows that a 3 day old job ("B") is not deleted,
+    whereas a 30 day old job ("A") is.
+    """
     mocker.patch("app.aws.s3.get_bucket_name", return_value="Bucket")
     mock_s3_client = mocker.Mock()
     mocker.patch("app.aws.s3.get_s3_client", return_value=mock_s3_client)
+    mock_remove_csv_object = mocker.patch("app.aws.s3.remove_csv_object")
+    lastmod30 = aware_utcnow() - timedelta(days=30)
+    lastmod3 = aware_utcnow() - timedelta(days=3)
 
     mock_s3_client.list_objects_v2.return_value = {
-        "Contents": [{"Key": "A", "LastModified": aware_utcnow()}]
+        "Contents": [
+            {"Key": "A", "LastModified": lastmod30},
+            {"Key": "B", "LastModified": lastmod3},
+        ]
     }
     cleanup_old_s3_objects()
     mock_s3_client.list_objects_v2.assert_called_with(Bucket="Bucket")
+    mock_remove_csv_object.assert_called_once_with("A")
 
 
 def test_get_s3_file_makes_correct_call(notify_api, mocker):