-
Notifications
You must be signed in to change notification settings - Fork 116
Use the s3fs profile argument as the boto3 profile name #985
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -205,10 +205,11 @@ def _get_client_compatible_with_s3fs(self, **kwargs) -> BaseClient: | |
|
|
||
| Accepts the constructor arguments that s3fs users pass through fsspec | ||
| storage options — ``key``/``username``, ``secret``/``password``, | ||
| ``token``, ``anon``, ``use_ssl``, ``endpoint_url``, | ||
| ``token``, ``profile``, ``anon``, ``use_ssl``, ``endpoint_url``, | ||
| ``connect_timeout``/``read_timeout``, and the ``client_kwargs`` / | ||
| ``config_kwargs`` dictionaries — in addition to boto3 session | ||
| arguments such as ``region_name`` and ``profile_name``. | ||
| arguments such as ``region_name`` and ``profile_name``. ``profile`` | ||
| is used as ``profile_name`` when ``profile_name`` is not given. | ||
|
|
||
| Args: | ||
| **kwargs: The filesystem constructor arguments. | ||
|
|
@@ -247,6 +248,8 @@ def _get_client_compatible_with_s3fs(self, **kwargs) -> BaseClient: | |
| } | ||
| kwargs.update(creds) | ||
| client_kwargs.update(creds) | ||
| if profile := kwargs.pop("profile", None): | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Self-review round one (implementation behavior, public contracts, simplicity, regression coverage) Base Covered inventory:
|
||
| kwargs.setdefault("profile_name", profile) | ||
|
|
||
| session = Session( | ||
| **{k: v for k, v in kwargs.items() if k in Connection._SESSION_PASSING_ARGS} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -205,6 +205,39 @@ def test_get_client_compatible_with_s3fs(self): | |
| ) | ||
| assert fs._client.meta.endpoint_url == "http://localhost:9000" | ||
|
|
||
| @pytest.mark.parametrize( | ||
| ("kwargs", "expected"), | ||
| [ | ||
| ({}, "DEFAULTKEY"), | ||
| # s3fs names the boto3 profile_name argument "profile". | ||
| ({"profile": "other"}, "OTHERKEY"), | ||
| ({"profile_name": "other"}, "OTHERKEY"), | ||
| ({"profile": "other", "profile_name": "default"}, "DEFAULTKEY"), | ||
| ], | ||
| ) | ||
| def test_get_client_compatible_with_s3fs_profile(self, monkeypatch, tmp_path, kwargs, expected): | ||
| # Only constructs a boto3 client from local profile files; no AWS access. | ||
| config = tmp_path / "config" | ||
| config.write_text("[default]\n[profile other]\n") | ||
| credentials = tmp_path / "credentials" | ||
| credentials.write_text( | ||
| "[default]\naws_access_key_id = DEFAULTKEY\naws_secret_access_key = secret\n" | ||
| "[other]\naws_access_key_id = OTHERKEY\naws_secret_access_key = secret\n" | ||
| ) | ||
| for name in ( | ||
| "AWS_PROFILE", | ||
| "AWS_DEFAULT_PROFILE", | ||
| "AWS_ACCESS_KEY_ID", | ||
| "AWS_SECRET_ACCESS_KEY", | ||
| "AWS_SESSION_TOKEN", | ||
| ): | ||
| monkeypatch.delenv(name, raising=False) | ||
| monkeypatch.setenv("AWS_CONFIG_FILE", str(config)) | ||
| monkeypatch.setenv("AWS_SHARED_CREDENTIALS_FILE", str(credentials)) | ||
|
|
||
| fs = S3FileSystem(region_name="us-east-1", skip_instance_cache=True, **kwargs) | ||
| assert fs._client._request_signer._credentials.access_key == expected | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Independent review (relayed reviewer result) Reviewer: OpenAI Codex CLI 0.160.0, model Result: CLEAN. Reviewer output:
|
||
|
|
||
| def test_ls_from_cache_with_cached_object(self): | ||
| fs = self._make_fs() | ||
| obj = S3Object( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-review round two (factual claims, caller compatibility, AWS operational effects, documentation)
Base
aa0fc9146f4e683d6cdf96b894f963f9dd8f7abe, head9cc6d013516cb94011ac159d99eddee851f8ea24. Result: CLEAN.Claims checked:
profile": s3fsS3FileSystemkeeps unknown constructor kwargs inself.kwargsand buildsaiobotocore.session.AioSession(**self.kwargs)(fsspec/s3fss3fs/core.py,set_session()), and its own docstring example isS3FileSystem(profile="<profile name>"). boto3Sessionnames itprofile_name(Connection._SESSION_PASSING_ARGS, connection.py:85-92). Holds.AioS3FileSystemandfsspec.filesystem("s3", profile=...)pick up the profile": checked with temporary profile files; both sign withOTHERKEY.profile_namewins when both are given": covered by the parametrized test case.ProfileNotFoundat construction, asprofile_name=did": checked forprofile=; both go through the sameSession(profile_name=...)call.profile_namewhenprofile_nameis not given": an explicitprofile_name=Nonecounts as given and keeps the default chain; acceptable and matches the wording.Existing callers: users who passed
profile=and relied on it being ignored now sign with that profile (or getProfileNotFound). This is the bug fix itself; listed in the PR body as a behavior change for the release notes.profile_name=callers andconnection=callers are unchanged.AWS operator: no new requests. Credential resolution for the profile (including assume-role or SSO profiles) is the same as for
profile_name=; botocore defers role assumption until the first request.Documentation: the new example sits with the other s3fs-compatible constructor arguments in
docs/filesystem.md;docs/usage.md:819covers connection-levelprofile_nameand stays correct. No other docs list the s3fs arguments.Evidence: offline test locally (5 passed; the
profilecase fails with the source change reverted). Live AWS tests not run locally; CI runs once Ready.