geruh commented on code in PR #2948:
URL: https://github.com/apache/iceberg-python/pull/2948#discussion_r2730151831
##########
pyiceberg/io/fsspec.py:
##########
@@ -205,7 +207,16 @@ def _s3(properties: Properties) -> AbstractFileSystem:
else:
anon = False
- fs = S3FileSystem(anon=anon, client_kwargs=client_kwargs,
config_kwargs=config_kwargs)
+ s3_fs_kwargs = {
+ "anon": anon,
+ "client_kwargs": client_kwargs,
+ "config_kwargs": config_kwargs,
+ }
+
+ if profile_name := get_first_property_value(properties, S3_PROFILE_NAME,
AWS_PROFILE_NAME):
+ s3_fs_kwargs["profile"] = profile_name
Review Comment:
Also, changes looks good!
The cred resolving chain looks as expected with `s3.profile-name` -->
`client.profile-name` using `get_first_property_value`, and you're only passing
the profile kwarg when it's actually set, which avoids the `profile=None`
issue. The Glue pattern matches what's already there for `region_name` and
`aws_access_key_id`, so that's consistent.
Tests cover both the fallback and resolving chain scenarios, which is what
matters. And thanks for testing with distinct IAM profiles in your actual
setup. That's alot more useful than unit tests alone for catching edge cases.
One thing to note is a user can set profile and credentials but credentials
should take precedence here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
