Skip to content
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

Jump to bottom

Full update options for Delta Sharing shares, recipients, and providers #530

Merged
merged 4 commits into from
Aug 12, 2022
Merged

Full update options for Delta Sharing shares, recipients, and providers #530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3ac99e9
.
wchau Aug 10, 2022
3086727
comments
wchau Aug 11, 2022
c5709c0
.
wchau Aug 12, 2022
0394144
Merge branch 'main' into ds_updates
pietern Aug 12, 2022
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: 15 additions & 3 deletions databricks_cli/unity_catalog/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
# limitations under the License.

from databricks_cli.unity_catalog.uc_service import UnityCatalogService
from databricks_cli.unity_catalog.utils import mc_pretty_format


class UnityCatalogApi(object):
Expand Down Expand Up @@ -224,9 +225,20 @@ def list_providers(self):
def get_provider(self, name):
return self.client.get_provider(name)

def update_provider(self, name, new_name=None, comment=None, recipient_profile=None):
return self.client.update_provider(name, new_name, comment, recipient_profile)

def update_provider(self, name, new_name=None, comment=None,
recipient_profile=None, provider_spec=None):
# If spec is provided, override all legacy parameters.
if provider_spec is not None:
return self.client.update_provider(name, provider_spec)
_data = {}
if new_name is not None:
_data['name'] = new_name
if recipient_profile is not None:
_data['recipient_profile_str'] = mc_pretty_format(recipient_profile)
if comment is not None:
_data['comment'] = comment
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we keeping the new_name, comment, recipient_profile args for backward compatibility (in case other apps call this function)? Looks like our CLI code only calls it with provider_spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The API is "public", so the test has been complaining about backwards incompatibility. This is to maintain the existing API.

return self.client.update_provider(name, _data)

def delete_provider(self, name):
return self.client.delete_provider(name)

Expand Down
96 changes: 74 additions & 22 deletions databricks_cli/unity_catalog/delta_sharing_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@
from databricks_cli.unity_catalog.api import UnityCatalogApi
from databricks_cli.unity_catalog.utils import hide, json_file_help, json_string_help, \
mc_pretty_format
from databricks_cli.utils import eat_exceptions, CONTEXT_SETTINGS, json_cli_base
from databricks_cli.utils import eat_exceptions, CONTEXT_SETTINGS, json_cli_base, \
merge_dicts_shallow


############## Share Commands ##############
Expand Down Expand Up @@ -127,6 +128,11 @@ def shared_data_object(name):
short_help='Update a share.')
@click.option('--name', required=True,
help='Name of the share to update.')
@click.option('--new-name', default=None, help='New name of the share.')
@click.option('--comment', default=None, required=False,
help='Free-form text description.')
@click.option('--owner', default=None, required=False,
help='Owner of the share.')
@click.option('--add-table', default=None, multiple=True,
metavar='NAME',
help='Full name of table to add to share (can be specified multiple times).')
Expand All @@ -141,20 +147,24 @@ def shared_data_object(name):
@profile_option
@eat_exceptions
@provide_api_client
def update_share_cli(api_client, name, add_table, remove_table, json_file, json):
def update_share_cli(api_client, name, new_name, comment, owner,
add_table, remove_table, json_file, json):
"""
Update a share.

The public specification for the JSON request is in development.
"""
if len(add_table) > 0 or len(remove_table) > 0:
if ((new_name is not None) or (comment is not None) or (owner is not None) or
len(add_table) > 0 or len(remove_table) > 0):
if (json_file is not None) or (json is not None):
raise ValueError('Cannot specify JSON if any other update flags are specified')
updates = []
for a in add_table:
updates.append({'action': 'ADD', 'data_object': shared_data_object(a)})
for r in remove_table:
updates.append({'action': 'REMOVE', 'data_object': shared_data_object(r)})
d = {'updates': updates}
share_json = UnityCatalogApi(api_client).update_share(name, d)
data = {'name': new_name, 'comment': comment, 'owner': owner, 'updates': updates}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we should filter out keys that have value None. Probably not needed now, since the server won't see those fields defined. We'll need to revisit this once the UC 'update' APIs allow clearing fields (with FieldMasks or whatever).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we can do that later, since all the other APIs do something similar along the lines.

share_json = UnityCatalogApi(api_client).update_share(name, data)
click.echo(mc_pretty_format(share_json))
else:
json_cli_base(json_file, json,
Expand Down Expand Up @@ -185,10 +195,10 @@ def delete_share_cli(api_client, name):
help='Free-form text description.')
@click.option('--sharing-id', default=None, required=False,
help='The sharing identifier provided by the data recipient offline.')
@click.option('--allowed_ip_address', default=None, required=False, multiple=True,
@click.option('--allowed-ip-address', default=None, required=False, multiple=True,
help=(
'IP address in CIDR notation that is allowed to use delta sharing. '
'Supports multiple options.'))
'(can be specified multiple times).'))
@debug_option
@profile_option
@eat_exceptions
Expand Down Expand Up @@ -236,6 +246,16 @@ def get_recipient_cli(api_client, name):
short_help='Update a recipient.')
@click.option('--name', required=True,
help='Name of the recipient who needs to be updated.')
@click.option('--new-name', default=None, help='New name of the recipient.')
@click.option('--comment', default=None, required=False,
help='Free-form text description.')
@click.option('--owner', default=None, required=False,
help='Owner of the recipient.')
@click.option('--allowed-ip-address', default=None, required=False, multiple=True,
help=(
'IP address in CIDR notation that is allowed to use delta sharing '
'(can be specified multiple times). Specify a single empty string to disable '
'IP allowlist.'))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bad API, IMO. If the intent is to clear the allowed IP addresses field to delete them, there should be a dedicated option, e.g. --clear-allowed-ip-addresses to do so. Using a magic empty string can lead to incorrect usage where the list is cleared accidentally (e.g. bad interpolation in a script).

@click.option('--json-file', default=None, type=click.Path(),
help=json_file_help(method='PATCH', path='/recipients/{name}'))
@click.option('--json', default=None, type=JsonClickType(),
Expand All @@ -244,14 +264,27 @@ def get_recipient_cli(api_client, name):
@profile_option
@eat_exceptions
@provide_api_client
def update_recipient_cli(api_client, name, json_file, json):
def update_recipient_cli(api_client, name, new_name, comment, owner,
allowed_ip_address, json_file, json):
"""
Update a recipient.

The public specification for the JSON request is in development.
"""
json_cli_base(json_file, json,
lambda json: UnityCatalogApi(api_client).update_recipient(name, json))
if ((new_name is not None) or (comment is not None) or (owner is not None) or
(allowed_ip_address is not None)):
if (json_file is not None) or (json is not None):
raise ValueError('Cannot specify JSON if any other update flags are specified')
data = {'name': new_name, 'comment': comment, 'owner': owner}
if len(allowed_ip_address) > 0:
data['ip_access_list'] = {}
if len(allowed_ip_address) != 1 or allowed_ip_address[0] != '':
data['ip_access_list']['allowed_ip_addresses'] = allowed_ip_address
recipient_json = UnityCatalogApi(api_client).update_recipient(name, data)
click.echo(mc_pretty_format(recipient_json))
else:
json_cli_base(json_file, json,
lambda json: UnityCatalogApi(api_client).update_recipient(name, json))


@click.command(context_settings=CONTEXT_SETTINGS,
Expand Down Expand Up @@ -369,34 +402,53 @@ def get_provider_cli(api_client, name):
@click.command(context_settings=CONTEXT_SETTINGS,
short_help='Update a provider.')
@click.option('--name', required=True, help='Name of the provider to update.')
@click.option('--new_name', default=None, help='New name of the provider.')
@click.option('--new-name', default=None, help='New name of the provider.')
@click.option('--comment', default=None, required=False,
help='Free-form text description.')
@click.option('--owner', default=None, required=False,
help='Owner of the provider.')
@click.option('--recipient-profile-json-file', default=None, type=click.Path(),
help='File containing recipient profile in JSON format.')
@click.option('--recipient-profile-json', default=None, type=JsonClickType(),
help='JSON string containing recipient profile.')
@click.option('--json-file', default=None, type=click.Path(),
help=json_file_help(method='PATCH', path='/providers/{name}'))
@click.option('--json', default=None, type=JsonClickType(),
help=json_string_help(method='PATCH', path='/providers/{name}'))
@debug_option
@profile_option
@eat_exceptions
@provide_api_client
def update_provider_cli(api_client, name, new_name, comment, recipient_profile_json_file,
recipient_profile_json):
def update_provider_cli(api_client, name, new_name, comment, owner, recipient_profile_json_file,
recipient_profile_json, json_file, json):
"""
Update a provider.

The public specification for the JSON request is in development.
"""
if recipient_profile_json is None and recipient_profile_json_file is None:
updated_provider = UnityCatalogApi(api_client).update_provider(name, new_name, comment)
click.echo(mc_pretty_format(updated_provider))
if ((new_name is not None) or (comment is not None) or (owner is not None) or
(recipient_profile_json_file is not None) or (recipient_profile_json) is not None):
if (json_file is not None) or (json is not None):
raise ValueError('Cannot specify JSON if any other update flags are specified')
data = {'name': new_name, 'comment': comment, 'owner': owner}

if (recipient_profile_json_file is None) and (recipient_profile_json is None):
provider_json = UnityCatalogApi(api_client).update_provider(name, provider_spec=data)
click.echo(mc_pretty_format(provider_json))
else:
json_cli_base(recipient_profile_json_file, recipient_profile_json,
lambda profile_json: UnityCatalogApi(api_client).update_provider(
name,
provider_spec=merge_dicts_shallow(
data,
{'recipient_profile_str': mc_pretty_format(profile_json)})),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can just set the recipient_profile_str key prior to this call. No need for a merge_dicts call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't set this easily without a nested def because it's scoped within a lambda function. With merge_dicts_shallow, it can be reused in the future in a easier fashion.

error_msg='Either --recipient-profile-json-file or ' +
'--recipient-profile-json should be provided')
else:
json_cli_base(recipient_profile_json_file, recipient_profile_json,
lambda json: UnityCatalogApi(api_client).update_provider(name, new_name,
comment, json),
error_msg='Either --recipient-profile-json-file or ' +
'--recipient-profile-json should be provided')

json_cli_base(json_file, json,
lambda json: UnityCatalogApi(api_client).update_provider(
name, provider_spec=json))


@click.command(context_settings=CONTEXT_SETTINGS,
short_help='List shares of a provider.')
Expand Down
12 changes: 2 additions & 10 deletions databricks_cli/unity_catalog/uc_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,17 +457,9 @@ def get_provider(self, name, headers=None):
return self.client.perform_query('GET', '/unity-catalog/providers/%s' % (name),
data={}, headers=headers)

def update_provider(self, name, new_name, comment, recipient_profile, headers=None):
_data = {}
if new_name is not None:
_data['name'] = new_name
if recipient_profile is not None:
_data['recipient_profile_str'] = mc_pretty_format(recipient_profile)
if comment is not None:
_data['comment'] = comment

def update_provider(self, name, provider_spec, headers=None):
return self.client.perform_query('PATCH', '/unity-catalog/providers/%s' % (name),
data=_data, headers=headers)
data=provider_spec, headers=headers)

def delete_provider(self, name, headers=None):
return self.client.perform_query('DELETE', '/unity-catalog/providers/%s' % (name),
Expand Down
11 changes: 11 additions & 0 deletions databricks_cli/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,3 +161,14 @@ def for_profile(profile):
'Please configure by entering '
'`{argv} configure --profile {profile}`').format(
profile=profile, argv=sys.argv[0]))


def merge_dicts_shallow(*dicts):
"""
Merges dicts through shallow copy.
"""
result = {}
for d in dicts:
result.update(d)
return result