feat(storage): implement support for storing artifacts to S3

[jkuri]

As discussed here I'm opening an implementation to support storing artifacts to S3 bucket.

[b-rowan]

I can dismiss this CodeQL alert if you want, the upload is already on a password protected section, and requires write permissions, so I think this is one that we can accept, since really only admins should be able to access this.

[jkuri]

I can dismiss this CodeQL alert if you want, the upload is already on a password protected section, and requires write permissions, so I think this is one that we can accept, since really only admins should be able to access this.

Yes, was trying to fix that error but so far without any success.

[b-rowan]

Couple of styling changes to fix, content looks fine to me though.

Only issue we might have, and this is something we should test against, is that I cant tell if this will break the Remote URL upload.

[b-rowan]

You may want to run pre-commit install in your venv, that way the checks stop complaining.

[jkuri]

Hi @b-rowan,

first of all - thank you for a detailed review, I updated and fixed all of it. Also, I tested both storage adapters with default and remote URL file as well and you were right - the S3 implementation did not worked for remote URL files but now it does.

I used https://share.jankuri.eu/output.swu for testing purposes, if maybe you'll test it yourself I'll leave this file online.

Please check the PR again when you'll have time and I'm opened to comments if there is still something needed to be fixed.

[b-rowan]

Small fix here, and I think the async with can be removed since we don't need to do any io when the storage starts. I have attached a patch that I tested with and found to work.

Subject: [PATCH] refactor: remove unneeded async with clause
---
Index: conftest.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/conftest.py b/conftest.py
--- a/conftest.py	(revision 0da04206ec16a18a47fac9ed26dbe67a4e044af1)
+++ b/conftest.py	(revision 5d459fd368c2c98b310cf19db045eda244365527)
@@ -35,7 +35,6 @@
 
 @pytest_asyncio.fixture(scope="function")
 async def test_app():
-    from goosebit.storage import storage
     from goosebit.users import create_initial_user
 
     async with RegisterTortoise(
@@ -45,8 +44,7 @@
         await Tortoise.generate_schemas()
         await create_initial_user(username="testing@goosebit.test", hashed_pwd=PWD_CXT.hash("test"))
 
-        async with storage:
-            yield app
+        yield app
 
 
 @pytest_asyncio.fixture(scope="function")
Index: goosebit/__init__.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/goosebit/__init__.py b/goosebit/__init__.py
--- a/goosebit/__init__.py	(revision 0da04206ec16a18a47fac9ed26dbe67a4e044af1)
+++ b/goosebit/__init__.py	(revision 5d459fd368c2c98b310cf19db045eda244365527)
@@ -16,7 +16,6 @@
 from goosebit import api, db, ui, updater
 from goosebit.auth import get_user_from_request, login_user, redirect_if_authenticated
 from goosebit.settings import PWD_CXT, config
-from goosebit.storage import storage
 from goosebit.ui.nav import nav
 from goosebit.ui.static import static
 from goosebit.ui.templates import templates
@@ -31,11 +30,10 @@
     if not db_ready:
         logger.exception("DB does not exist, try running `poetry run aerich upgrade`.")
 
-    async with storage:
-        logger.debug(f"Initialized storage backend: {config.storage.backend}")
+    logger.debug(f"Initialized storage backend: {config.storage.backend}")
 
-        if db_ready:
-            yield
+    if db_ready:
+        yield
 
     await db.close()
 
Index: goosebit/storage/__init__.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/goosebit/storage/__init__.py b/goosebit/storage/__init__.py
--- a/goosebit/storage/__init__.py	(revision 0da04206ec16a18a47fac9ed26dbe67a4e044af1)
+++ b/goosebit/storage/__init__.py	(revision 5d459fd368c2c98b310cf19db045eda244365527)
@@ -12,7 +12,7 @@
 class GoosebitStorage:
     def __init__(self, config: GooseBitSettings):
         self.config = config
-        self._backend: StorageProtocol | None = None
+        self._backend: StorageProtocol = self._create_backend()
 
     def _create_backend(self) -> StorageProtocol:
 
@@ -35,13 +35,6 @@
         else:
             raise ValueError(f"Unknown storage backend type: {self.config.storage.backend}")
 
-    async def __aenter__(self) -> "GoosebitStorage":
-        self._backend = self._create_backend()
-        return self
-
-    async def __aexit__(self, *_) -> None:
-        self._backend = None
-
     @property
     def backend(self) -> StorageProtocol:
         if self._backend is None:

[jkuri]

@b-rowan seems like the patches you sent are already applied. I fixed that path concatenation in create_software_update function.

[easybe]

Not a big fan of the hard dependency on the AWS SDK.

Can't we make it optional like we did for Postgres?

[jkuri]

I have no objections making it optional.

[b-rowan]

Sounds good to me. I'll throw something together here quickly.