From f452d37629301c5e2e1a93080b3825d8f1ee4404 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Mon, 16 Dec 2024 14:15:13 -0800 Subject: [PATCH] Address feedback by using report_zip directly as IO[byte] stream and move bucket lifecycle configuration to terraform. --- .../generate_inventory_and_hold_reports.py | 29 +++------ src/palace/manager/service/storage/s3.py | 57 ----------------- ...est_generate_inventory_and_hold_reports.py | 2 - tests/manager/service/storage/test_s3.py | 63 ------------------- 4 files changed, 10 insertions(+), 141 deletions(-) diff --git a/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py b/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py index 5a238acf9..7453f561d 100644 --- a/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py +++ b/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py @@ -148,25 +148,16 @@ def run(self) -> None: arcname=f"palace-inventory-report-for-library-{file_name_modifier}.csv", ) - reports_path = "inventory_and_holds" - expiration_in_days = 30 - self.s3_service.update_bucket_expiration_rule( - prefix=f"{reports_path}/", expiration_in_days=expiration_in_days + uid = uuid.uuid4() + key = ( + f"inventory_and_holds/{library.short_name}/" + f"inventory-and-holds-for-library-{file_name_modifier}-{uid}.zip" + ) + self.s3_service.store_stream( + key, + report_zip, # type: ignore[arg-type] + content_type="application/zip", ) - - with zip_path.open( - "rb", - ) as binary_stream: - uid = uuid.uuid4() - key = ( - f"{reports_path}/{library.short_name}/" - f"inventory-and-holds-for-library-{file_name_modifier}-{uid}.zip" - ) - self.s3_service.store_stream( - key, - binary_stream, - content_type="application/zip", - ) s3_file_link = self.s3_service.generate_url(key) self.send_email( @@ -174,7 +165,7 @@ def run(self) -> None: receivers=[self.email_address], text=( f"Download Report here -> {s3_file_link} \n\n" - f"This report will be available for download for {expiration_in_days} days." + f"This report will be available for download for 30 days." ), ) diff --git a/src/palace/manager/service/storage/s3.py b/src/palace/manager/service/storage/s3.py index 290fff066..d68941f21 100644 --- a/src/palace/manager/service/storage/s3.py +++ b/src/palace/manager/service/storage/s3.py @@ -244,60 +244,3 @@ def multipart_abort(self, key: str, upload_id: str) -> None: Key=key, UploadId=upload_id, ) - - def update_bucket_expiration_rule( - self, prefix: str, expiration_in_days: int = 30 - ) -> None: - """ - Update the expiration lifecycle policy rule if it exists and has changed. If - it does not already exist, add one. - """ - rule_name = f"expiration_on_{prefix}" - bucket = self.bucket - configuration = self.client.get_bucket_lifecycle_configuration(Bucket=bucket) - - rules_list = configuration["Rules"] - for rule in rules_list: - if "ID" in rule and rule["ID"] == rule_name: - if ( - rule["Prefix"] == prefix - and rule["Expiration"]["Days"] == expiration_in_days - ): - self.log.info( - f"Expiration lifecycle rule {rule_name} has not changed for {bucket}: " - f"current value = {rule}. Ignoring." - ) - return - else: - # remove the old version of the rule from the list - rules_list.remove(rule) - break - - rules_list.append( - { - "Expiration": { - "Days": expiration_in_days, - }, - "ID": rule_name, - "Prefix": prefix, - "Filter": { - "Prefix": prefix, - }, - "Status": "Enabled", - } - ) - - try: - policy_status = self.client.put_bucket_lifecycle_configuration( - Bucket=bucket, LifecycleConfiguration={"Rules": rules_list} # type: ignore[typeddict-item] - ) - - self.log.info( - f"Updated bucket lifecycle configuration for {bucket}: " - f"configuration={configuration}, " - f"status = {policy_status}" - ) - except ClientError as e: - self.log.error( - f"Unable to apply bucket lifecycle configuration for {self.bucket}. \nReason:{e}" - ) diff --git a/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py b/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py index af7d1f2ac..1f5908ecf 100644 --- a/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py +++ b/tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py @@ -210,7 +210,6 @@ def store_stream_mock( job.run() - mock_s3.update_bucket_expiration_rule.assert_called_once() mock_s3.generate_url.assert_called_once() send_email_mock.assert_called_once() kwargs = send_email_mock.call_args.kwargs @@ -319,7 +318,6 @@ def test_generate_inventory_and_hold_reports_task( services_fixture.email_fixture.mock_emailer.send.assert_called_once() mock_s3_service.store_stream.assert_called_once() - mock_s3_service.update_bucket_expiration_rule.assert_called_once() mock_s3_service.generate_url.assert_called_once() assert ( diff --git a/tests/manager/service/storage/test_s3.py b/tests/manager/service/storage/test_s3.py index 80497917a..f8265266f 100644 --- a/tests/manager/service/storage/test_s3.py +++ b/tests/manager/service/storage/test_s3.py @@ -240,69 +240,6 @@ def _configuration(self, prefix, expiration): ] } - def test_update_bucket_expiration_rule_not_previously_set( - self, s3_service_fixture: S3ServiceFixture - ): - service = s3_service_fixture.service() - prefix = "prefix/" - expiration_in_days = 10 - - s3_service_fixture.mock_s3_client.get_bucket_lifecycle_configuration.return_value = { - "Rules": [] - } - service.update_bucket_expiration_rule( - prefix=prefix, expiration_in_days=expiration_in_days - ) - s3_service_fixture.mock_s3_client.get_bucket_lifecycle_configuration.assert_called_once() - put_config_method = ( - s3_service_fixture.mock_s3_client.put_bucket_lifecycle_configuration - ) - put_config_method.assert_called_once_with( - Bucket="bucket", - LifecycleConfiguration=self._configuration(prefix, expiration_in_days), - ) - - def test_update_bucket_expiration_rule_previously_set_unchanged( - self, s3_service_fixture: S3ServiceFixture - ): - service = s3_service_fixture.service() - prefix = "prefix/" - expiration_in_days = 10 - - get_bucket_lifecycle_config = ( - s3_service_fixture.mock_s3_client.get_bucket_lifecycle_configuration - ) - get_bucket_lifecycle_config.return_value = self._configuration( - prefix, expiration_in_days - ) - service.update_bucket_expiration_rule( - prefix=prefix, expiration_in_days=expiration_in_days - ) - s3_service_fixture.mock_s3_client.get_bucket_lifecycle_configuration.assert_called_once() - s3_service_fixture.mock_s3_client.put_bucket_lifecycle_configuration.assert_not_called() - - def test_update_bucket_expiration_rule_previously_set_changed( - self, s3_service_fixture: S3ServiceFixture - ): - service = s3_service_fixture.service() - prefix = "prefix/" - expiration_in_days = 10 - get_lifecycle = ( - s3_service_fixture.mock_s3_client.get_bucket_lifecycle_configuration - ) - get_lifecycle.return_value = self._configuration(prefix, 12) - service.update_bucket_expiration_rule( - prefix=prefix, expiration_in_days=expiration_in_days - ) - s3_service_fixture.mock_s3_client.get_bucket_lifecycle_configuration.assert_called_once() - put_config_method = ( - s3_service_fixture.mock_s3_client.put_bucket_lifecycle_configuration - ) - put_config_method.assert_called_once_with( - Bucket="bucket", - LifecycleConfiguration=self._configuration(prefix, expiration_in_days), - ) - @pytest.mark.minio class TestS3ServiceIntegration: