Copilot commented on code in PR #2948: URL: https://github.com/apache/iceberg-python/pull/2948#discussion_r2725699847
########## tests/io/test_fsspec_profile.py: ########## @@ -0,0 +1,86 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + + +import uuid +from unittest import mock + +from pyiceberg.io.fsspec import FsspecFileIO +from pyiceberg.typedef import Properties + +UNIFIED_AWS_SESSION_PROPERTIES = { + "client.access-key-id": "client.access-key-id", + "client.secret-access-key": "client.secret-access-key", + "client.region": "client.region", + "client.session-token": "client.session-token", +} Review Comment: The constant `UNIFIED_AWS_SESSION_PROPERTIES` is being redefined locally instead of importing it from `tests.conftest`, which deviates from the established pattern in the codebase. Multiple test files (e.g., `tests/catalog/test_glue.py`, `tests/catalog/test_dynamodb.py`, `tests/io/test_pyarrow.py`) import this constant from `tests.conftest`. Consider importing it from `tests.conftest` to maintain consistency and avoid duplication. ########## tests/catalog/test_glue_profile.py: ########## @@ -0,0 +1,51 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from unittest import mock + +from moto import mock_aws + +from pyiceberg.catalog.glue import GlueCatalog +from pyiceberg.typedef import Properties + +UNIFIED_AWS_SESSION_PROPERTIES = { + "client.access-key-id": "client.access-key-id", + "client.secret-access-key": "client.secret-access-key", + "client.region": "client.region", + "client.session-token": "client.session-token", +} + + +@mock_aws +def test_passing_client_profile_name_properties_to_glue() -> None: + session_properties: Properties = { + "client.profile-name": "profile_name", + **UNIFIED_AWS_SESSION_PROPERTIES, + } + + with mock.patch("boto3.Session") as mock_session: + test_catalog = GlueCatalog("glue", **session_properties) + + mock_session.assert_called_with( + aws_access_key_id="client.access-key-id", + aws_secret_access_key="client.secret-access-key", + aws_session_token="client.session-token", + region_name="client.region", + profile_name="profile_name", + botocore_session=None, + ) + assert test_catalog.glue is mock_session().client() Review Comment: Missing test case for precedence when both `glue.profile-name` and `client.profile-name` are provided. According to the implementation and pattern established in the codebase, `glue.profile-name` should take precedence over `client.profile-name`. A test should verify that when both properties are set, `glue.profile-name` is used. ########## tests/io/test_fsspec.py: ########## @@ -34,7 +34,13 @@ from pyiceberg.io.fsspec import FsspecFileIO, S3V4RestSigner from pyiceberg.io.pyarrow import PyArrowFileIO from pyiceberg.typedef import Properties -from tests.conftest import UNIFIED_AWS_SESSION_PROPERTIES + +UNIFIED_AWS_SESSION_PROPERTIES = { + "client.access-key-id": "client.access-key-id", + "client.secret-access-key": "client.secret-access-key", + "client.region": "client.region", + "client.session-token": "client.session-token", +} Review Comment: The constant `UNIFIED_AWS_SESSION_PROPERTIES` is being redefined locally instead of importing it from `tests.conftest`, which deviates from the established pattern in the codebase. Other test files in the same directory (e.g., `tests/io/test_pyarrow.py`) import this constant from `tests.conftest`. Consider importing it from `tests.conftest` to maintain consistency and avoid duplication. ########## pyiceberg/io/__init__.py: ########## @@ -41,12 +41,14 @@ logger = logging.getLogger(__name__) +AWS_PROFILE_NAME = "client.profile-name" AWS_REGION = "client.region" AWS_ACCESS_KEY_ID = "client.access-key-id" AWS_SECRET_ACCESS_KEY = "client.secret-access-key" AWS_SESSION_TOKEN = "client.session-token" AWS_ROLE_ARN = "client.role-arn" AWS_ROLE_SESSION_NAME = "client.role-session-name" +S3_PROFILE_NAME = "s3.profile-name" Review Comment: The documentation for Unified AWS Credentials (mkdocs/docs/configuration.md lines 808-831) should be updated to include the newly added `client.profile-name` property. The table starting at line 823 is missing an entry for `client.profile-name` to document that it sets the AWS profile for both the catalog and S3 FileIO. ```suggestion S3_PROFILE_NAME = AWS_PROFILE_NAME ``` ########## tests/io/test_fsspec_profile.py: ########## @@ -0,0 +1,86 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + + +import uuid +from unittest import mock + +from pyiceberg.io.fsspec import FsspecFileIO +from pyiceberg.typedef import Properties + +UNIFIED_AWS_SESSION_PROPERTIES = { + "client.access-key-id": "client.access-key-id", + "client.secret-access-key": "client.secret-access-key", + "client.region": "client.region", + "client.session-token": "client.session-token", +} + + +def test_fsspec_s3_session_properties_with_profile() -> None: + session_properties: Properties = { + "s3.profile-name": "test-profile", + "s3.endpoint": "http://localhost:9000";, + **UNIFIED_AWS_SESSION_PROPERTIES, + } + + with mock.patch("s3fs.S3FileSystem") as mock_s3fs, mock.patch("aiobotocore.session.AioSession") as mock_aio_session: + s3_fileio = FsspecFileIO(properties=session_properties) + filename = str(uuid.uuid4()) + + s3_fileio.new_input(location=f"s3://warehouse/{filename}") + + mock_aio_session.assert_called_with(profile="test-profile") + mock_s3fs.assert_called_with( + anon=False, + client_kwargs={ + "endpoint_url": "http://localhost:9000";, + "aws_access_key_id": "client.access-key-id", + "aws_secret_access_key": "client.secret-access-key", + "region_name": "client.region", + "aws_session_token": "client.session-token", + }, + config_kwargs={}, + session=mock_aio_session(), + ) + + +def test_fsspec_s3_session_properties_with_client_profile() -> None: + session_properties: Properties = { + "client.profile-name": "test-profile", + "s3.endpoint": "http://localhost:9000";, + **UNIFIED_AWS_SESSION_PROPERTIES, + } + + with mock.patch("s3fs.S3FileSystem") as mock_s3fs, mock.patch("aiobotocore.session.AioSession") as mock_aio_session: + s3_fileio = FsspecFileIO(properties=session_properties) + filename = str(uuid.uuid4()) + + s3_fileio.new_input(location=f"s3://warehouse/{filename}") + + mock_aio_session.assert_called_with(profile="test-profile") + mock_s3fs.assert_called_with( + anon=False, + client_kwargs={ + "endpoint_url": "http://localhost:9000";, + "aws_access_key_id": "client.access-key-id", + "aws_secret_access_key": "client.secret-access-key", + "region_name": "client.region", + "aws_session_token": "client.session-token", + }, + config_kwargs={}, + session=mock_aio_session(), + ) Review Comment: Missing test case for precedence when both `s3.profile-name` and `client.profile-name` are provided. According to the PR description, `s3.profile-name` should take precedence over `client.profile-name`. A test should verify that when both properties are set, `s3.profile-name` is used. ########## tests/catalog/test_glue_profile.py: ########## @@ -0,0 +1,51 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from unittest import mock + +from moto import mock_aws + +from pyiceberg.catalog.glue import GlueCatalog +from pyiceberg.typedef import Properties + +UNIFIED_AWS_SESSION_PROPERTIES = { + "client.access-key-id": "client.access-key-id", + "client.secret-access-key": "client.secret-access-key", + "client.region": "client.region", + "client.session-token": "client.session-token", +} Review Comment: The constant `UNIFIED_AWS_SESSION_PROPERTIES` is being redefined locally instead of importing it from `tests.conftest`, which deviates from the established pattern in the codebase. Multiple test files (e.g., `tests/catalog/test_glue.py`, `tests/catalog/test_dynamodb.py`) import this constant from `tests.conftest`. Consider importing it from `tests.conftest` to maintain consistency and avoid duplication. ########## pyiceberg/io/__init__.py: ########## @@ -41,12 +41,14 @@ logger = logging.getLogger(__name__) +AWS_PROFILE_NAME = "client.profile-name" AWS_REGION = "client.region" AWS_ACCESS_KEY_ID = "client.access-key-id" AWS_SECRET_ACCESS_KEY = "client.secret-access-key" AWS_SESSION_TOKEN = "client.session-token" AWS_ROLE_ARN = "client.role-arn" AWS_ROLE_SESSION_NAME = "client.role-session-name" +S3_PROFILE_NAME = "s3.profile-name" Review Comment: The S3 FileIO configuration documentation (mkdocs/docs/configuration.md lines 112-131) should be updated to include the newly added `s3.profile-name` property. The table should include an entry for `s3.profile-name` to document that it sets the AWS profile specifically for S3 FileIO operations. -- 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]
