Add optional folder argument for file uploads to S3 Buckets #208
Conversation
This update introduces an optional folder argument, allowing users to specify the target location within the S3 bucket for file uploads.
|
@raverburg thank you for your contribution! As this is your first code contribution, please read the following Contributor License Agreement (CLA). If you agree with the CLA, please reply with the following information:
Contributor License Agreement
Contribution License AgreementThis Contribution License Agreement ("Agreement") governs your Contribution(s) (as defined below) and conveys certain license rights to Fox-IT B.V. ("Fox-IT") for your Contribution(s) to Fox-IT"s open source Dissect project. This Agreement covers any and all Contributions that you ("You" or "Your"), now or in the future, Submit (as defined below) to this project. This Agreement is between Fox-IT B.V. and You and takes effect when you click an “I Accept” button, check box presented with these terms, otherwise accept these terms or, if earlier, when You Submit a Contribution.
|
|
@DissectBot agree |
acquire/uploaders/minio.py
Outdated
| @@ -20,6 +20,7 @@ def __init__(self, upload: dict[str, str], **kwargs: dict[str, Any]) -> None: | |||
| self.access_id = upload.get("access_id") | |||
| self.access_key = upload.get("access_key") | |||
| self.bucket_name = upload.get("bucket") | |||
| self.folder = upload.get("folder", "") | |||
There was a problem hiding this comment.
| self.folder = upload.get("folder", "") | |
| self.folder = upload.get("folder", "").rstrip("/") |
acquire/uploaders/minio.py
Outdated
| if self.folder: | ||
| object_path = f"{self.folder.rstrip('/')}/{os.path.basename(path)}" | ||
| else: | ||
| object_path = os.path.basename(path) |
There was a problem hiding this comment.
| if self.folder: | |
| object_path = f"{self.folder.rstrip('/')}/{os.path.basename(path)}" | |
| else: | |
| object_path = os.path.basename(path) | |
| object_path = path.name | |
| if self.folder: | |
| object_path = f"{self.folder}/{object_path}" |
Done! It's been a while since I last wrote tests, so please feel free to correct me if this is not what you meant. :) |
There was a problem hiding this comment.
Technically, all the return types of the test functions should return a -> None but this file has not changed in a bit, so I won't ask you to do that ;)
After you made the changes, it might be a good idea to run tox run -e fix to fix any formatting issues. It will prevent the scenario that the linting part of our pipeline fails :)
tests/test_minio_uploader.py
Outdated
| minio.folder = "Uploads/test_folder" | ||
| assert minio.folder == "Uploads/test_folder" |
There was a problem hiding this comment.
This part only tests the assignment of the folder attribute, not what happens during the __init__ of the object. I think this would test the thing you where after
| minio.folder = "Uploads/test_folder" | |
| assert minio.folder == "Uploads/test_folder" | |
| arguments["folder"] = "Uploads/test_folder" | |
| minio_no_backslash = minio_plugin(upload=arguments) | |
| assert minio_no_backslash.folder == "Uploads/test_folder" |
There was a problem hiding this comment.
You're totally right, this is what I meant to do ;)
tests/test_minio_uploader.py
Outdated
| @@ -79,3 +79,46 @@ def test_upload_file_multiple_failures(minio_instance: MinIO): | |||
| with patch("acquire.uploaders.plugin.log") as mocked_logger: | |||
| upload_files_using_uploader(minio_instance, [Path("hello")]) | |||
| mocked_logger.error.assert_called_with("Upload %s FAILED after too many attempts. Stopping.", Path("hello")) | |||
|
|
|||
| def test_minio_folder_initialization(minio_plugin): | |||
There was a problem hiding this comment.
| def test_minio_folder_initialization(minio_plugin): | |
| def test_minio_folder_initialization(minio_plugin: Callable) -> None: |
Callable should probably be imported at the top first.
| assert minio_no_folder.folder == "" | ||
|
|
||
|
|
||
| def test_upload_file_with_folder(minio_instance: MinIO): |
There was a problem hiding this comment.
| def test_upload_file_with_folder(minio_instance: MinIO): | |
| def test_upload_file_with_folder(minio_instance: MinIO) -> None: |
tests/test_minio_uploader.py
Outdated
| mock_client = Mock() | ||
| minio_instance.folder = "" | ||
| mock_client = Mock() | ||
| test_path = Path("example.txt") |
There was a problem hiding this comment.
| mock_client = Mock() | |
| minio_instance.folder = "" | |
| mock_client = Mock() | |
| test_path = Path("example.txt") | |
| mock_client = Mock() | |
| test_path = Path("example.txt") | |
| minio_instance.folder = "" |
for consistency with the other test :)
This is what I meant, thanks for the tests! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #208 +/- ##
==========================================
+ Coverage 47.38% 47.42% +0.04%
==========================================
Files 26 26
Lines 3438 3441 +3
==========================================
+ Hits 1629 1632 +3
Misses 1809 1809
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
there is only one more issue, with an unused import :) |
Oops.. should be good now :D |
A new feature to add an optional folder argument, allowing users to specify the target location within the S3 bucket for file uploads. This creates flexability for users wanting to specify where acquire uploads the files.
Example config:
{ "arguments": [ ], "public_key": "", "upload": { "mode": "cloud", "endpoint": "s3.amazonaws.com", "access_id": "", "access_key": "", "bucket": "example", "folder":"Evidence/Uploads" } }