Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add MINIO_STORAGE_REGION setting #130

Merged
merged 1 commit into from
May 31, 2023
Merged

Conversation

krystofbe
Copy link

This pull request aims to implement the addition of the MINIO_STORAGE_REGION setting, which was discussed in detail in the GitHub issue here.

To ensure testing, I have created a new TestCase called SettingsTests since I didn't find a good place for the tests. However, I am encountering some failures in the Tox tests on my pull request branch, and I am not entirely familiar with Tox to determine the cause of these failures.

@thomasf
Copy link
Collaborator

thomasf commented May 30, 2023

hmm. I was able to push a small fix to your branch but this PR didn't update.

Squash everything into a commit with a single commit message and we are ready to go.

@thomasf
Copy link
Collaborator

thomasf commented May 30, 2023

Now I don't see those commits anymore..

in any case I just made this adjustment

diff --git a/minio_storage/storage.py b/minio_storage/storage.py
index 00a853e..97582f3 100644
--- a/minio_storage/storage.py
+++ b/minio_storage/storage.py
@@ -364,12 +364,17 @@ def get_setting(name: str, default=_NoValue) -> T.Any:


 def create_minio_client_from_settings(*, minio_kwargs=None):
-    minio_kwargs = minio_kwargs or {}
+    kwargs = {}
     endpoint = get_setting("MINIO_STORAGE_ENDPOINT")
     access_key = get_setting("MINIO_STORAGE_ACCESS_KEY")
     secret_key = get_setting("MINIO_STORAGE_SECRET_KEY")
     secure = get_setting("MINIO_STORAGE_USE_HTTPS", True)
     region = get_setting("MINIO_STORAGE_REGION", None)
+    if region:
+        kwargs["region"] = region
+
+    if minio_kwargs:
+        kwargs.update(minio_kwargs)
     # Making this client deconstructible allows it to be passed directly as
     # an argument to MinioStorage, since Django needs to be able to
     # deconstruct all Storage constructor arguments for Storages referenced in
@@ -379,8 +384,7 @@ def create_minio_client_from_settings(*, minio_kwargs=None):
         access_key=access_key,
         secret_key=secret_key,
         secure=secure,
-        region=region,
-        **minio_kwargs,
+        **kwargs,
     )
     return client

@krystofbe
Copy link
Author

Squash everything into a commit with a single commit message and we are ready to go.

Jättebra! It's one commit now: 113365e

@thomasf
Copy link
Collaborator

thomasf commented May 31, 2023

you need to make the change to how kwargs are applied above for the tests to pass as well.

@thomasf
Copy link
Collaborator

thomasf commented May 31, 2023

https://github.com/py-pa/django-minio-storage/blob/master/tests/test_app/tests/custom_storage_class_tests.py#L49 that example fails otherwise due to double region keyword args being applied.

@krystofbe
Copy link
Author

https://github.com/py-pa/django-minio-storage/blob/master/tests/test_app/tests/custom_storage_class_tests.py#L49 that example fails otherwise due to double region keyword args being applied.

ok got the tests running now locally and removed the kwargs attribute

@thomasf
Copy link
Collaborator

thomasf commented May 31, 2023

make this change instead #130 (comment) so we don't break backwards compatibility with examples in the source code.

@krystofbe
Copy link
Author

Ok. Pushed again. Can you tell me why tox errors? The error looks rather cryptic to me:

  File "/home/runner/work/django-minio-storage/django-minio-storage/.tox/.pkg/lib/python3.11/site-packages/setuptools_scm/version.py", line 203, in _parse_tag
    assert version is not None
           ^^^^^^^^^^^^^^^^^^^
AssertionError

@thomasf thomasf merged commit 7782818 into py-pa:master May 31, 2023
@thomasf
Copy link
Collaborator

thomasf commented May 31, 2023

Nope., git fetch --tags maybe.. idk.

For me it generates this version py39-django42-minioknown: install_package> python -I -m pip install --force-reinstall --no-deps /home/t/src/github.com/py-pa/django-minio-storage/.tox/.tmp/package/393/django-minio-storage-0.5.5.dev2+g176c9cb.tar.gz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants