Skip to content

Fix polaris CLI support for updating catalog properties and hitting real Polaris services #404

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.

Sign up for GitHub

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

Merged
eric-maynard merged 8 commits into from
Oct 30, 2024
Merged

Fix polaris CLI support for updating catalog properties and hitting real Polaris services #404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
59ef275
Fix polaris CLI support for updating catalog properties and hitting r…
dennishuo Oct 25, 2024
59c2d9a
Small whitespace consistency fix.
dennishuo Oct 25, 2024
6eca851
Also update test_cli_parsing
dennishuo Oct 25, 2024
927dffc
Introduce --set-property and --remove-property for all "update" comma…
dennishuo Oct 29, 2024
1d11373
Also add --remove-property to cli parsing test
dennishuo Oct 29, 2024
d1d45e1
Merge branch 'main' of github.com:dennishuo/polaris into dhuo-fix-pyt…
dennishuo Oct 29, 2024
aed4a8c
Also test parsing invalid --set-property on create
dennishuo Oct 30, 2024
adb0898
Merge branch 'main' of github.com:dennishuo/polaris into dhuo-fix-pyt…
dennishuo Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions regtests/client/python/cli/command/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ def options_get(key, f=lambda x: x):
return f(getattr(options, key)) if hasattr(options, key) else None

properties = Parser.parse_properties(options_get(Arguments.PROPERTY))
set_properties = Parser.parse_properties(options_get(Arguments.SET_PROPERTY))
remove_properties = options_get(Arguments.REMOVE_PROPERTY)

command = None
if options.command == Commands.CATALOGS:
Expand All @@ -57,7 +59,9 @@ def options_get(key, f=lambda x: x):
consent_url=options_get(Arguments.CONSENT_URL),
service_account=options_get(Arguments.SERVICE_ACCOUNT),
catalog_name=options_get(Arguments.CATALOG),
properties={} if properties is None else properties
properties={} if properties is None else properties,
set_properties={} if set_properties is None else set_properties,
remove_properties=[] if remove_properties is None else remove_properties
)
elif options.command == Commands.PRINCIPALS:
from cli.command.principals import PrincipalsCommand
Expand All @@ -67,7 +71,9 @@ def options_get(key, f=lambda x: x):
principal_name=options_get(Arguments.PRINCIPAL),
client_id=options_get(Arguments.CLIENT_ID),
principal_role=options_get(Arguments.PRINCIPAL_ROLE),
properties=properties
properties={} if properties is None else properties,
set_properties={} if set_properties is None else set_properties,
remove_properties=[] if remove_properties is None else remove_properties
)
elif options.command == Commands.PRINCIPAL_ROLES:
from cli.command.principal_roles import PrincipalRolesCommand
Expand All @@ -77,7 +83,9 @@ def options_get(key, f=lambda x: x):
principal_name=options_get(Arguments.PRINCIPAL),
catalog_name=options_get(Arguments.CATALOG),
catalog_role_name=options_get(Arguments.CATALOG_ROLE),
properties=properties
properties={} if properties is None else properties,
set_properties={} if set_properties is None else set_properties,
remove_properties=[] if remove_properties is None else remove_properties
)
elif options.command == Commands.CATALOG_ROLES:
from cli.command.catalog_roles import CatalogRolesCommand
Expand All @@ -86,7 +94,9 @@ def options_get(key, f=lambda x: x):
catalog_name=options_get(Arguments.CATALOG),
catalog_role_name=options_get(Arguments.CATALOG_ROLE),
principal_role_name=options_get(Arguments.PRINCIPAL_ROLE),
properties=properties
properties={} if properties is None else properties,
set_properties={} if set_properties is None else set_properties,
remove_properties=[] if remove_properties is None else remove_properties
)
elif options.command == Commands.PRIVILEGES:
from cli.command.privileges import PrivilegesCommand
Expand Down
17 changes: 15 additions & 2 deletions regtests/client/python/cli/command/catalog_roles.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
# under the License.
#
from dataclasses import dataclass
from typing import Dict, Optional
from typing import Dict, Optional, List

from pydantic import StrictStr

Expand Down Expand Up @@ -46,6 +46,8 @@ class CatalogRolesCommand(Command):
catalog_role_name: str
principal_role_name: str
properties: Optional[Dict[str, StrictStr]]
Copy link
Contributor

@eric-maynard eric-maynard Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does properties do now? Can we remove it?

Copy link
Contributor

@eric-maynard eric-maynard Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e. could you use set-property during create as well?

Copy link

@sfc-gh-dhuo sfc-gh-dhuo Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking for preserving --property for create is to emphasize the contrasting "declarative" semantics of CREATE vs the choice of having an "imperative" semantic for UPDATE. Looking at examples of other CLIs with a similar split, the same pattern is used:

gcloud compute instances create ... --labels=...
gcloud compute instances update ... --update-labels=... --remove-labels=...

And similarly --update-labels and --remove-labels aren't available in the create command, while --labels is not available in the update command.

Copy link
Contributor

@eric-maynard eric-maynard Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. My initial hesitation was just having two properties that do the same thing.

Lets keep this, but I think we should reject the "invalid" inputs then:
catalogs update some_catalog --property k=v
catalogs update some_catalog --set-property k=v

Copy link

@sfc-gh-dhuo sfc-gh-dhuo Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, this test case in test_cli_parsing.py covers the rejection of --property:

https://github.com/apache/polaris/pull/404/files#diff-437c53a66ce1039f3ff70bef07bb0325924edf2b8f2c864762969e6ecdde3ecfR58

    with self.assertRaises(SystemExit) as cm:
        Parser.parse(['catalogs', 'update', 'catalog_name', '--property', 'foo=bar'])
    self.assertEqual(cm.exception.code, INVALID_ARGS)

set_properties: Dict[str, StrictStr]
remove_properties: List[str]

def validate(self):
if not self.catalog_name:
Expand Down Expand Up @@ -77,9 +79,20 @@ def execute(self, api: PolarisDefaultApi) -> None:
print(catalog_role.to_json())
elif self.catalog_roles_subcommand == Subcommands.UPDATE:
catalog_role = api.get_catalog_role(self.catalog_name, self.catalog_role_name)
new_properties = catalog_role.properties or {}

# Add or update all entries specified in set_properties
if self.set_properties:
new_properties = {**new_properties, **self.set_properties}

# Remove all keys specified in remove_properties
if self.remove_properties:
for to_remove in self.remove_properties:
new_properties.pop(to_remove, None)

request = UpdateCatalogRoleRequest(
current_entity_version=catalog_role.entity_version,
properties=self.properties
properties=new_properties
)
api.update_catalog_role(self.catalog_name, self.catalog_role_name, request)
elif self.catalog_roles_subcommand == Subcommands.GRANT:
Expand Down
32 changes: 24 additions & 8 deletions regtests/client/python/cli/command/catalogs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ class CatalogsCommand(Command):
service_account: str
catalog_name: str
properties: Dict[str, StrictStr]
set_properties: Dict[str, StrictStr]
remove_properties: List[str]

def validate(self):
if self.catalogs_subcommand == Subcommands.CREATE:
Expand Down Expand Up @@ -176,22 +178,36 @@ def execute(self, api: PolarisDefaultApi) -> None:
print(catalog.to_json())
elif self.catalogs_subcommand == Subcommands.UPDATE:
catalog = api.get_catalog(self.catalog_name)
if self.default_base_location or self.properties:

if self.default_base_location or self.set_properties or self.remove_properties:
new_default_base_location = self.default_base_location or catalog.properties.default_base_location
new_additional_properties = catalog.properties.additional_properties or {}

# Add or update all entries specified in set_properties
if self.set_properties:
new_additional_properties = {**new_additional_properties, **self.set_properties}

# Remove all keys specified in remove_properties
if self.remove_properties:
for to_remove in self.remove_properties:
new_additional_properties.pop(to_remove, None)

catalog.properties = CatalogProperties(
default_base_location=self.default_base_location,
additional_properties=self.properties
default_base_location=new_default_base_location,
additional_properties=new_additional_properties
)
request = UpdateCatalogRequest(
current_entity_version=catalog.entity_version,
catalog=catalog
)
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):
request = UpdateCatalogRequest(
current_entity_version=catalog.entity_version,
catalog=catalog,
properties=catalog.properties.to_dict(),
storage_config_info=self._build_storage_config_info()
)
else:
request = UpdateCatalogRequest(
current_entity_version=catalog.entity_version,
properties=catalog.properties.to_dict()
)

api.update_catalog(self.catalog_name, request)
else:
Expand Down
13 changes: 7 additions & 6 deletions regtests/client/python/cli/command/namespaces.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ def _get_catalog_api(self, api: PolarisDefaultApi):
"""
Convert a management API to a catalog API
"""
catalog_host = re.match(r'(http://[^/]+)', api.api_client.configuration.host).group(1)
catalog_host = re.match(r'(https?://.+)/api/management', api.api_client.configuration.host).group(1)
configuration = Configuration(
host=f'{catalog_host}/api/catalog',
username=api.api_client.configuration.username,
Expand All @@ -71,12 +71,12 @@ def _get_catalog_api(self, api: PolarisDefaultApi):
def execute(self, api: PolarisDefaultApi) -> None:
catalog_api = self._get_catalog_api(api)
if self.namespaces_subcommand == Subcommands.CREATE:
properties = self.properties or {}
req_properties = self.properties or {}
if self.location:
properties = {**properties, Arguments.LOCATION: self.location}
req_properties = {**req_properties, Arguments.LOCATION: self.location}
request = CreateNamespaceRequest(
namespace=self.namespace,
properties=self.properties
properties=req_properties
)
catalog_api.create_namespace(
prefix=self.catalog,
Expand All @@ -91,7 +91,8 @@ def execute(self, api: PolarisDefaultApi) -> None:
elif self.namespaces_subcommand == Subcommands.DELETE:
catalog_api.drop_namespace(prefix=self.catalog, namespace=UNIT_SEPARATOR.join(self.namespace))
elif self.namespaces_subcommand == Subcommands.GET:
catalog_api.namespace_exists(prefix=self.catalog, namespace=UNIT_SEPARATOR.join(self.namespace))
print(json.dumps({"namespace": '.'.join(self.namespace)}))
print(catalog_api.load_namespace_metadata(
prefix=self.catalog,
namespace=UNIT_SEPARATOR.join(self.namespace)).to_json())
else:
raise Exception(f"{self.namespaces_subcommand} is not supported in the CLI")
17 changes: 15 additions & 2 deletions regtests/client/python/cli/command/principal_roles.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
# under the License.
#
from dataclasses import dataclass
from typing import Dict, Optional
from typing import Dict, Optional, List

from pydantic import StrictStr

Expand Down Expand Up @@ -46,6 +46,8 @@ class PrincipalRolesCommand(Command):
catalog_name: str
catalog_role_name: str
properties: Optional[Dict[str, StrictStr]]
set_properties: Dict[str, StrictStr]
remove_properties: List[str]

def validate(self):
if self.principal_roles_subcommand == Subcommands.LIST:
Expand Down Expand Up @@ -82,9 +84,20 @@ def execute(self, api: PolarisDefaultApi) -> None:
print(principal_role.to_json())
elif self.principal_roles_subcommand == Subcommands.UPDATE:
principal_role = api.get_principal_role(self.principal_role_name)
new_properties = principal_role.properties or {}

# Add or update all entries specified in set_properties
if self.set_properties:
new_properties = {**new_properties, **self.set_properties}

# Remove all keys specified in remove_properties
if self.remove_properties:
for to_remove in self.remove_properties:
new_properties.pop(to_remove, None)

request = UpdatePrincipalRoleRequest(
current_entity_version=principal_role.entity_version,
properties=self.properties
properties=new_properties
)
api.update_principal_role(self.principal_role_name, request)
elif self.principal_roles_subcommand == Subcommands.GRANT:
Expand Down
19 changes: 16 additions & 3 deletions regtests/client/python/cli/command/principals.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
# under the License.
#
from dataclasses import dataclass
from typing import Dict, Optional
from typing import Dict, Optional, List

from pydantic import StrictStr

Expand Down Expand Up @@ -45,6 +45,8 @@ class PrincipalsCommand(Command):
client_id: str
principal_role: str
properties: Optional[Dict[str, StrictStr]]
set_properties: Dict[str, StrictStr]
remove_properties: List[str]

def validate(self):
pass
Expand Down Expand Up @@ -75,9 +77,20 @@ def execute(self, api: PolarisDefaultApi) -> None:
print(api.rotate_credentials(self.principal_name).to_json())
elif self.principals_subcommand == Subcommands.UPDATE:
principal = api.get_principal(self.principal_name)
new_properties = principal.properties or {}

# Add or update all entries specified in set_properties
if self.set_properties:
new_properties = {**new_properties, **self.set_properties}

# Remove all keys specified in remove_properties
if self.remove_properties:
for to_remove in self.remove_properties:
new_properties.pop(to_remove, None)

request = UpdatePrincipalRequest(
current_entity_version=principal.current_entity_version,
properties=self.properties
current_entity_version=principal.entity_version,
properties=new_properties
)
api.update_principal(self.principal_name, request)
else:
Expand Down
12 changes: 12 additions & 0 deletions regtests/client/python/cli/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,8 @@ class Arguments:
CLIENT_ID = 'client_id'
PRINCIPAL_ROLE = 'principal_role'
PROPERTY = 'property'
SET_PROPERTY = 'set_property'
REMOVE_PROPERTY = 'remove_property'
PRIVILEGE = 'privilege'
NAMESPACE = 'namespace'
TABLE = 'table'
Expand All @@ -126,6 +128,7 @@ class Arguments:
ACCESS_TOKEN = 'access_token'
HOST = 'host'
PORT = 'port'
BASE_URL = 'base_url'
PARENT = 'parent'
LOCATION = 'location'

Expand All @@ -139,6 +142,13 @@ class Hints:

PROPERTY = ('A key/value pair such as: tag=value. Multiple can be provided by specifying this option'
' more than once')
SET_PROPERTY = ('A key/value pair such as: tag=value. Merges the specified key/value into an existing'
' properties map by updating the value if the key already exists or creating a new'
' entry if not. Multiple can be provided by specifying this option more than once')
REMOVE_PROPERTY = ('A key to remove from a properties map. If the key already does not exist then'
' no action is takn for the specified key. If properties are also being set in'
' the same update command then the list of removals is applied last. Multiple'
' can be provided by specifying this option more than once')

class Catalogs:
GRANT = 'Grant a catalog role to a catalog'
Expand Down Expand Up @@ -217,3 +227,5 @@ class Namespaces:
UNIT_SEPARATOR = chr(0x1F)
CLIENT_ID_ENV = 'CLIENT_ID'
CLIENT_SECRET_ENV = 'CLIENT_SECRET'
DEFAULT_HOSTNAME = 'localhost'
DEFAULT_PORT = 8181
12 changes: 8 additions & 4 deletions regtests/client/python/cli/options/option_tree.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,8 @@ def get_tree() -> List[Option]:
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.PROPERTY, str, Hints.PROPERTY, allow_repeats=True),
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)
]),
Option(Commands.PRINCIPALS, 'manage principals', children=[
Expand All @@ -118,7 +119,8 @@ def get_tree() -> List[Option]:
Option(Subcommands.LIST),
Option(Subcommands.ROTATE_CREDENTIALS, input_name=Arguments.PRINCIPAL),
Option(Subcommands.UPDATE, args=[
Argument(Arguments.PROPERTY, str, Hints.PROPERTY, allow_repeats=True)
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.PRINCIPAL)
]),
Option(Commands.PRINCIPAL_ROLES, 'manage principal roles', children=[
Expand All @@ -132,7 +134,8 @@ def get_tree() -> List[Option]:
Argument(Arguments.PRINCIPAL, str, Hints.PrincipalRoles.List.PRINCIPAL_NAME)
]),
Option(Subcommands.UPDATE, args=[
Argument(Arguments.PROPERTY, str, Hints.PROPERTY, allow_repeats=True)
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.PRINCIPAL_ROLE),
Option(Subcommands.GRANT, hint=Hints.PrincipalRoles.GRANT, args=[
Argument(Arguments.PRINCIPAL, str, Hints.PrincipalRoles.Grant.PRINCIPAL)
Expand All @@ -157,7 +160,8 @@ def get_tree() -> List[Option]:
], input_name=Arguments.CATALOG),
Option(Subcommands.UPDATE, args=[
Argument(Arguments.CATALOG, str, Hints.CatalogRoles.CATALOG_NAME),
Argument(Arguments.PROPERTY, str, Hints.PROPERTY, allow_repeats=True)
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_ROLE),
Option(Subcommands.GRANT, hint=Hints.CatalogRoles.GRANT_CATALOG_ROLE, args=[
Argument(Arguments.CATALOG, str, Hints.CatalogRoles.CATALOG_NAME),
Expand Down
5 changes: 3 additions & 2 deletions regtests/client/python/cli/options/parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,9 @@ class Parser(object):
"""

_ROOT_ARGUMENTS = [
Argument(Arguments.HOST, str, hint='hostname', default='localhost'),
Argument(Arguments.PORT, int, hint='port', default=8181),
Argument(Arguments.HOST, str, hint='hostname'),
Argument(Arguments.PORT, int, hint='port'),
Argument(Arguments.BASE_URL, str, hint='complete base URL instead of hostname:port'),
Argument(Arguments.CLIENT_ID, str, hint='client ID for token-based authentication'),
Argument(Arguments.CLIENT_SECRET, str, hint='client secret for token-based authentication'),
Argument(Arguments.ACCESS_TOKEN, str, hint='access token for token-based authentication'),
Expand Down
Loading