Merge branch 'feature/misc-wb-cleanup' into feature/post-buff-worms

 Closes #465
 [ENG-8558]

Author: felliott

tests/providers/s3/test_provider.py

@@ -9,10 +9,7 @@
 from http import client
 from urllib import parse
 from unittest import mock
-
 import pytest
-# from boto.compat import BytesIO
-# from boto.utils import compute_md5
 
 from waterbutler.providers.s3 import S3Provider
 from waterbutler.core.path import WaterButlerPath
@@ -151,8 +148,6 @@ def list_upload_chunks_body(parts_metadata):
         </ListPartsResult>
     '''
 
-    # md5 = compute_md5(BytesIO(payload))
-    # md5 = compute_md5(payload)
     md5 = hashlib.md5(payload)
 
     headers = {
@@ -169,7 +164,10 @@ def build_folder_params(path):
 
 class TestRegionDetection:
 
-    @pytest.mark.skip('TODO fix broken s3 provider tests')
+    @pytest.mark.skip('S3 test suite tests are skipped because many of them are incompatible with the recent '
+                      'migration from the older `boto` library to `boto3`. The underlying mechanisms for request signing'
+                      'and client interaction have changed significantly, breaking the old test logic. The suite '
+                      'needs to be refactored to work properly with the current S3 providers implementation.')
     @pytest.mark.asyncio
     @pytest.mark.aiohttpretty
     @pytest.mark.parametrize("region_name,expected_region", [
@@ -196,23 +194,6 @@ async def test_region_host(self, auth, credentials, settings, region_name, expec
         aiohttpretty.register_uri('GET', region_url, status=200, body=location_response(region_name))
         await provider._check_region()
         assert provider.region == expected_region
-        # provider = S3Provider(auth, credentials, settings)
-        # await provider._check_region()
-        # res = await provider._get_bucket_region()
-        # # region_url = provider.bucket.generate_url(
-        # #     100,
-        # #     'GET',
-        # #     query_parameters={'location': ''},
-        # # )
-        # region_url = 'https://s3.amazonaws.com/that-kerning?location=&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=Dont%20dead%2F20250526%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20250526T134653Z&X-Amz-Expires=100&X-Amz-SignedHeaders=host&X-Amz-Signature=80f8426c4fc6d0af68bd3e52a553c9e4d838144b9a70600aff507f70056696f1 '
-        # aiohttpretty.register_uri('GET',
-        #                           region_url,
-        #                           status=200,
-        #                           body=location_response(region_name))
-        #
-        # await provider._check_region()
-        # assert provider.connection.host == host
-
 
 class TestValidatePath:
 

tests/tasks/conftest.py

@@ -14,25 +14,6 @@
 logger = logging.getLogger(__name__)
 
 
-# Task testing doesn't play nice with unittest-style tests, so fallback to regular pytest-style
-# setup/teardown.
-
-# def pytest_runtest_setup(item):
-#     print(f'@@@ pytest setup alpha')
-#     policy = asyncio.get_event_loop_policy()
-#     policy.get_event_loop().close()
-#     event_loop = policy.new_event_loop()
-#     policy.set_event_loop(event_loop)
-#     logger.error(f'@@@ pytest setup beta')
-
-
-# def pytest_runtest_teardown(item):
-#     logger.error(f'@@@ pytest teardown gamma')
-#     policy = asyncio.get_event_loop_policy()
-#     policy.get_event_loop().close()
-#     logger.error(f'@@@ pytest teardown delta')
-
-
 # Fixtures for task tests
 
 @pytest.fixture(autouse=True)

tests/utils.py

@@ -199,11 +199,6 @@ def tearDown(self):
     def get_app(self):
         return make_app(debug=False)
 
-    # def get_new_ioloop(self):
-    #     loop = asyncio.new_event_loop()
-    #     asyncio.set_event_loop(loop)
-    #     return loop
-
 
 class MultiProviderHandlerTestCase(HandlerTestCase):
 

waterbutler/providers/s3/provider.py

@@ -130,7 +130,14 @@ async def get_s3_bucket_object_location(self):
             )
             return resp
 
-    # Todo:  the commented solution may be more stable than not commented
+    # NOTE: These commented-out blocks are left as a reference to an
+    # alternative implementation.
+    # The current active implementation was chosen to maintain unification
+    # with the generic `make_request` pattern used all over providers (see PR #463 for discussion).
+    # A trade-off is that the current approach requires using `unquote` on the path to handle xml properly,
+    # which this paginator-based solution might avoid, however maybe there is more explicit solution without `unquote`
+    # for 'make_requests' to keep it more explicit.
+    #
     # async def get_folder_metadata(self, path, params):
     #     try:
     #         contents, prefixes = [], []

waterbutler/server/api/v0/core.py

@@ -46,7 +46,6 @@ class BaseHandler(server_utils.CORsMixin, server_utils.UtilMixin, tornado.web.Re
     ACTION_MAP = {}  # type: dict
 
     def write_error(self, status_code, exc_info):
-        # TODO: maybe it is needed to change there too somehow
         sentry_sdk.capture_exception(exc_info)
         etype, exc, _ = exc_info
 

waterbutler/server/api/v1/core.py

@@ -3,7 +3,6 @@
 import tornado.web
 import tornado.gen
 import tornado.iostream
-
 import sentry_sdk
 
 from waterbutler import tasks
@@ -20,7 +19,6 @@ def as_entry(cls):
         return cls.PATTERN, cls
 
     def write_error(self, status_code, exc_info):
-        # TODO: maybe it is needed to change there too somehow
         etype, exc, _ = exc_info
 
         finish_args = []