Conversation
brianhelba
force-pushed
the
minio-kwargs
branch
2 times, most recently
from
5e630b6 to
535b484
Compare
brianhelba
changed the title
Minio as keyword argumentsminio as keyword arguments
brianhelba
force-pushed
the
minio-kwargs
branch
3 times, most recently
from
d60f74c to
696675f
Compare
| user_metadata = HTTPHeaderDict() | ||
| if self.object_metadata: | ||
| for metadata_key, metadata_value in self.object_metadata.items(): | ||
| if isinstance(metadata_value, str): | ||
| user_metadata.add(metadata_key, metadata_value) | ||
| else: | ||
| for metadata_value_element in metadata_value: | ||
| user_metadata.add(metadata_key, metadata_value_element) |
There was a problem hiding this comment.
Note, this change due to minio/minio-py#1534
I considered changing the type of MinioStorage.object_metadata as a breaking change. However, exposing that type as urllib3.HTTPHeaderDict seems less intuitive as an API for object metadata.
Note, the type of minio.Object.metadata is now Optional[Union[dict[str, str], HTTPHeaderDict]] (which I haven't tested and don't really know how to interpret), but that's not being exposed as part of our public API.
| def fmt(o: Object) -> str: | ||
| if template is None: | ||
| return o.object_name | ||
| return o.object_name or "" |
There was a problem hiding this comment.
Small bugfix here, detected by type checking.
brianhelba
force-pushed
the
minio-kwargs
branch
from
696675f to
be5bfa2
Compare
| raise CommandError(f"bucket {bucket_name} does not exist") from err | ||
| elif err.code == "NoSuchBucketPolicy": | ||
| raise CommandError(f"bucket {bucket_name} has no policy") from err | ||
| raise |
There was a problem hiding this comment.
Small bugfix here, detected by type checking.
brianhelba
force-pushed
the
minio-kwargs
branch
2 times, most recently
from
dd43ab8 to
80e0b9e
Compare
| user_metadata = HTTPHeaderDict() | ||
| if self.object_metadata: | ||
| for metadata_key, metadata_value in self.object_metadata.items(): | ||
| if isinstance(metadata_value, str): |
There was a problem hiding this comment.
I'm not sure how defensive to be here. We clearly type annotate that object_metadata must be a T.Mapping[str, str | Sequence[str]].
However, if someone did pass {'foo': 5} as metadata, I'm not sure if we should attempt to coerce that to a string or at least fail in a more obvious way.
Since str itself is a Sequence, I could check:
if isinstance(metadata_value, Sequence) and not isinstance(metadata_value, str)
| logger = getLogger("minio_storage") | ||
|
|
||
| ObjectMetadataType = T.Mapping[str, T.Union[str, list[str], tuple[str]]] | ||
| ObjectMetadataType = T.Mapping[str, T.Union[str, Sequence[str]]] |
There was a problem hiding this comment.
Slightly widened this, now that we're not bound by any upstream compatibility.
brianhelba
force-pushed
the
minio-kwargs
branch
2 times, most recently
from
37c7a2d to
dfda05e
Compare
| def create_minio_client_from_settings(*, minio_kwargs=None): | ||
| endpoint = get_setting("MINIO_STORAGE_ENDPOINT") | ||
| kwargs = { | ||
| def create_minio_client_from_settings(**minio_kwargs: T.Any) -> minio.Minio: |
There was a problem hiding this comment.
This is a slightly breaking change this function signature, but I think it results in a more sensible API. This will be released as part of a new minor version bump anyway.
|
@thomasf I'd appreciate a review on this. I'm disappointed by upstream |
brianhelba
force-pushed
the
minio-kwargs
branch
from
dfda05e to
a8670e8
Compare
brianhelba
changed the title
minio as keyword argumentsThis version now requires all arguments to be passed as keyword arguments. Additionally, the signature of `Minio.put_metadata` has changed (`metadata` is now `user_metadata`), so at least v7.2.19 is now strictly required.
brianhelba
force-pushed
the
minio-kwargs
branch
from
a8670e8 to
e2439c5
Compare
2 participants
This version now requires all arguments to be passed as keyword arguments.
Additionally, the signature of
Minio.put_metadatahas changed (metadatais nowuser_metadata), so at least v7.2.19 is now strictly required.