This is an automated email from the ASF dual-hosted git repository.
dhuo pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/polaris.git
The following commit(s) were added to refs/heads/main by this push:
new 16bc7fde Add support for updating catalog region, fix catalogs update,
fix running client pytests (#988)
16bc7fde is described below
commit 16bc7fde7d917f30838a73ea104a3de01d51aa9d
Author: Dennis Huo <[email protected]>
AuthorDate: Wed Feb 12 14:56:43 2025 -0800
Add support for updating catalog region, fix catalogs update, fix running
client pytests (#988)
* Add support for updating catalog region, fix catalogs update, fix running
client pytests
At head, currently "polaris catalogs update" is broken whenever
storage_config is
modified; just like updating properties or other top-level fields, the
storage_config
must be initially built off of the existing storage_config, and edits must
be applied
client-side.
This fixes --allowed-location and --region in "catalogs update".
Additionally, there was a recent regression where a "fix" in run.sh got rid
of
previously "accidentally" running the base directory and thus pulling in
client/python/test.
Now, make run.sh explicitly run the client/python/test directory to get all
the pytests in there
as a special case aside from all the t_* tests.
* Remove redundant parens
---
regtests/client/python/cli/command/catalogs.py | 32 ++++++++++++----
regtests/client/python/cli/options/option_tree.py | 1 +
regtests/client/python/test/test_cli_parsing.py | 7 ++++
regtests/run.sh | 19 +++++++++-
regtests/t_cli/src/test_cli.py | 46 +++++++++++++++++++++++
5 files changed, 96 insertions(+), 9 deletions(-)
diff --git a/regtests/client/python/cli/command/catalogs.py
b/regtests/client/python/cli/command/catalogs.py
index 9ed14609..eb8f4892 100644
--- a/regtests/client/python/cli/command/catalogs.py
+++ b/regtests/client/python/cli/command/catalogs.py
@@ -69,11 +69,6 @@ class CatalogsCommand(Command):
if not self.default_base_location:
raise Exception(f'Missing required argument:'
f'
{Argument.to_flag_name(Arguments.DEFAULT_BASE_LOCATION)}')
- if self.catalogs_subcommand == Subcommands.UPDATE:
- if self.allowed_locations:
- if not self.storage_type:
- raise Exception(f'Missing required argument when updating
allowed locations for a catalog:'
- f'
{Argument.to_flag_name(Arguments.STORAGE_TYPE)}')
if self.storage_type == StorageType.S3.value:
if not self.role_arn:
@@ -199,12 +194,33 @@ class CatalogsCommand(Command):
default_base_location=new_default_base_location,
additional_properties=new_additional_properties
)
- if (self._has_aws_storage_info() or self._has_azure_storage_info()
or self._has_gcs_storage_info() or
- self.allowed_locations or self.default_base_location):
+
+ if (self._has_aws_storage_info() or self._has_azure_storage_info()
or
+ self._has_gcs_storage_info() or self.allowed_locations):
+ # We must first reconstitute local storage-config related
settings from the existing
+ # catalog to properly construct the complete updated
storage-config
+ updated_storage_info = catalog.storage_config_info
+
+ # In order to apply mutations client-side, we can't just use
the base
+ # _build_storage_config_info helper; instead, each allowed
updatable field defined
+ # in option_tree.py should be applied individually against the
existing
+ # storage_config_info here.
+ if self.allowed_locations:
+
updated_storage_info.allowed_locations.extend(self.allowed_locations)
+
+ if self.region:
+ # Note: We have to lowercase the returned value because
the server enum
+ # is uppercase but we defined the StorageType enums as
lowercase.
+ storage_type = updated_storage_info.storage_type
+ if storage_type.lower() != StorageType.S3.value:
+ raise Exception(
+ f'--region requires S3 storage_type, got:
{storage_type}')
+ updated_storage_info.region = self.region
+
request = UpdateCatalogRequest(
current_entity_version=catalog.entity_version,
properties=catalog.properties.to_dict(),
- storage_config_info=self._build_storage_config_info()
+ storage_config_info=updated_storage_info
)
else:
request = UpdateCatalogRequest(
diff --git a/regtests/client/python/cli/options/option_tree.py
b/regtests/client/python/cli/options/option_tree.py
index dfbd642e..d048b1a5 100644
--- a/regtests/client/python/cli/options/option_tree.py
+++ b/regtests/client/python/cli/options/option_tree.py
@@ -105,6 +105,7 @@ class OptionTree:
Argument(Arguments.DEFAULT_BASE_LOCATION, str,
Hints.Catalogs.Update.DEFAULT_BASE_LOCATION),
Argument(Arguments.ALLOWED_LOCATION, str,
Hints.Catalogs.Create.ALLOWED_LOCATION,
allow_repeats=True),
+ Argument(Arguments.REGION, str,
Hints.Catalogs.Create.REGION),
Argument(Arguments.SET_PROPERTY, str, Hints.SET_PROPERTY,
allow_repeats=True),
Argument(Arguments.REMOVE_PROPERTY, str,
Hints.REMOVE_PROPERTY, allow_repeats=True),
], input_name=Arguments.CATALOG)
diff --git a/regtests/client/python/test/test_cli_parsing.py
b/regtests/client/python/test/test_cli_parsing.py
index 41e87afe..b9967ce9 100644
--- a/regtests/client/python/test/test_cli_parsing.py
+++ b/regtests/client/python/test/test_cli_parsing.py
@@ -283,6 +283,13 @@ class TestCliParsing(unittest.TestCase):
'get_catalog', {
(0, None): 'foo',
})
+ check_arguments(
+ mock_execute([
+ 'catalogs', 'update', 'foo', '--set-property', 'key=value',
+ '--default-base-location', 'x', '--region', 'us-west-1']),
+ 'get_catalog', {
+ (0, None): 'foo',
+ })
check_arguments(
mock_execute(['principals', 'create', 'foo', '--property',
'key=value']),
'create_principal', {
diff --git a/regtests/run.sh b/regtests/run.sh
index 2a9eb4aa..652bee0e 100755
--- a/regtests/run.sh
+++ b/regtests/run.sh
@@ -52,7 +52,7 @@ cd ${REGTEST_HOME}
if [ -z "${1}" ]; then
loginfo 'Running all tests'
- TEST_LIST="$(find t_* -wholename '*t_*/src/*')"
+ TEST_LIST="client/python/test $(find t_* -wholename '*t_*/src/*')"
else
loginfo "Running single test ${1}"
TEST_LIST=${1}
@@ -87,6 +87,21 @@ export REGTEST_ROOT_BEARER_TOKEN=$token
echo "Root bearer token: ${REGTEST_ROOT_BEARER_TOKEN}"
for TEST_FILE in ${TEST_LIST}; do
+ # Special-case running all client pytests
+ if [ "${TEST_FILE}" == 'client/python/test' ]; then
+ loginfo "Starting pytest for entire client suite"
+ python3 -m pytest ${TEST_FILE}
+ CODE=$?
+ if [[ $CODE -ne 0 ]]; then
+ logred "Test FAILED: ${TEST_FILE}"
+ NUM_FAILURES=$(( NUM_FAILURES + 1 ))
+ else
+ loggreen "Test SUCCEEDED: ${TEST_FILE}"
+ fi
+ continue
+ fi
+
+ # Handle individually-specified pytests
TEST_SUITE=$(dirname $(dirname ${TEST_FILE}))
TEST_SHORTNAME=$(basename ${TEST_FILE})
if [[ "${TEST_SHORTNAME}" =~ .*.py ]]; then
@@ -105,6 +120,8 @@ for TEST_FILE in ${TEST_LIST}; do
fi
continue
fi
+
+ # Assume anything else is open-ended executable-script based test
if [[ "${TEST_SHORTNAME}" =~ .*.azure.*.sh ]]; then
if [ -z "${AZURE_CLIENT_ID}" ] || [ -z "${AZURE_CLIENT_SECRET}" ] || [
-z "${AZURE_TENANT_ID}" ] ; then
loginfo "Azure tests not enabled, skip running test ${TEST_FILE}"
diff --git a/regtests/t_cli/src/test_cli.py b/regtests/t_cli/src/test_cli.py
index 876a53e8..02071875 100644
--- a/regtests/t_cli/src/test_cli.py
+++ b/regtests/t_cli/src/test_cli.py
@@ -373,6 +373,52 @@ def test_update_catalog():
checker=lambda s: 'foo' not in s and 'prop2' not in s and
'"prop3": "3333"' in s and '"prop4":
"4444"' in s)
+ # Update to set a region
+ check_output(root_cli(
+ 'catalogs',
+ 'update',
+ f'test_cli_catalog_{SALT}',
+ '--region',
+ 'new-test-region'
+ ), checker=lambda s: s == '')
+
+ # Original fake-location should still be present in
storage_config_info.allowed_locations
+ check_output(root_cli('catalogs', 'get', f'test_cli_catalog_{SALT}'),
+ checker=lambda s: 's3://fake-location-' in s and
'"region": "new-test-region"' in s)
+
+ # Update to add an allowed location
+ check_output(root_cli(
+ 'catalogs',
+ 'update',
+ f'test_cli_catalog_{SALT}',
+ '--allowed-location',
+ f's3://extra-allowed-location-{SALT}'
+ ), checker=lambda s: s == '')
+
+ # All allowed locations present and region still present
+ check_output(root_cli('catalogs', 'get', f'test_cli_catalog_{SALT}'),
+ checker=lambda s: 's3://fake-location-' in s and
+ 's3://extra-allowed-location-' in s and
+ '"region": "new-test-region"' in s)
+
+ # Add allowed location and change region at same time
+ check_output(root_cli(
+ 'catalogs',
+ 'update',
+ f'test_cli_catalog_{SALT}',
+ '--allowed-location',
+ f's3://fourth-allowed-location-{SALT}',
+ '--region',
+ 'us-east-2'
+ ), checker=lambda s: s == '')
+
+ # All allowed locations present and new region set
+ check_output(root_cli('catalogs', 'get', f'test_cli_catalog_{SALT}'),
+ checker=lambda s: 's3://fake-location-' in s and
+ 's3://extra-allowed-location-' in s and
+ 's3://fourth-allowed-location-' in s and
+ '"region": "us-east-2"' in s)
+
finally:
sys.path.pop(0)
pass
