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

wchau · 2022-08-02T06:30:42Z

Added support for all updates to shares, recipients, and providers.
Allow option to either specify individual fields to update or JSON.

Tested locally.

codecov-commenter · 2022-08-02T06:33:17Z

Codecov Report

Merging #530 (0394144) into main (ef918f1) will decrease coverage by 0.42%.
The diff coverage is 1.69%.

@@            Coverage Diff             @@
##             main     #530      +/-   ##
==========================================
- Coverage   60.72%   60.29%   -0.43%     
==========================================
  Files          55       55              
  Lines        4692     4727      +35     
==========================================
+ Hits         2849     2850       +1     
- Misses       1843     1877      +34

Impacted Files	Coverage Δ
databricks_cli/unity_catalog/api.py	`0.00% <0.00%> (ø)`
databricks_cli/unity_catalog/delta_sharing_cli.py	`0.00% <0.00%> (ø)`
databricks_cli/unity_catalog/uc_service.py	`0.00% <0.00%> (ø)`
databricks_cli/utils.py	`86.45% <20.00%> (-3.66%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

adamcain-db

Basically LGTM. Just a couple questions.

adamcain-db · 2022-08-02T16:01:04Z

databricks_cli/unity_catalog/api.py

+        if recipient_profile is not None:
+            _data['recipient_profile_str'] = mc_pretty_format(recipient_profile)
+        if comment is not None:
+            _data['comment'] = comment


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.

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

adamcain-db · 2022-08-02T16:03:48Z

databricks_cli/unity_catalog/delta_sharing_cli.py

        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}


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).

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

adamcain-db

LGTM. Might want to wait til @pietern or team member can have a look before merging.

pietern · 2022-08-11T13:36:42Z

databricks_cli/unity_catalog/delta_sharing_cli.py

@@ -188,7 +197,7 @@ def delete_share_cli(api_client, name):
 @click.option('--allowed_ip_address', default=None, required=False, multiple=True,


@wchau Could you change this to use dashes to be consistent with everything else and check if other commands use the same? We shouldn't end up in a situation where folks depend on it.

pietern · 2022-08-11T13:36:48Z

databricks_cli/unity_catalog/delta_sharing_cli.py

+              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,


pietern · 2022-08-11T13:37:49Z

databricks_cli/utils.py

+
+def merge_dicts(*dicts):
+    """
+    Merges dicts throw shallow copy.


*through

Please rename this to merge_dicts_shallow to reflect the shallow nature at the call site.

No need for this function at all, per the other comment.

Per other comment

pietern · 2022-08-11T13:39:27Z

databricks_cli/unity_catalog/delta_sharing_cli.py

+                              name,
+                              provider_spec=merge_dicts(
+                                  data,
+                                  {'recipient_profile_str': mc_pretty_format(profile_json)})),


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

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.

pietern

Thanks for the update. Please consider addressing the concern w.r.t. --allowed-ip-address in the update operation in a follow up PR. No need to block a release for this to be addressed.

pietern · 2022-08-12T08:50:18Z

databricks_cli/unity_catalog/delta_sharing_cli.py

+              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.'))


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).

wchau requested review from adamcain-db and pietern August 2, 2022 06:30

adamcain-db reviewed Aug 2, 2022

View reviewed changes

adamcain-db approved these changes Aug 2, 2022

View reviewed changes

wchau requested a review from fjakobs August 3, 2022 22:05

wchau force-pushed the ds_updates branch from 4bcc8bd to 26d1283 Compare August 4, 2022 18:04

.

3ac99e9

wchau force-pushed the ds_updates branch from 26d1283 to 3ac99e9 Compare August 10, 2022 20:38

pietern reviewed Aug 11, 2022

View reviewed changes

wchau requested a review from pietern August 11, 2022 20:49

comments

3086727

wchau force-pushed the ds_updates branch from bf74c76 to 3086727 Compare August 11, 2022 21:02

wchau and others added 2 commits August 11, 2022 20:47

.

c5709c0

Merge branch 'main' into ds_updates

0394144

pietern approved these changes Aug 12, 2022

View reviewed changes

pietern merged commit 136e429 into databricks:main Aug 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

wchau commented Aug 2, 2022

codecov-commenter commented Aug 2, 2022 •

edited

Loading

adamcain-db left a comment

adamcain-db Aug 2, 2022

wchau Aug 2, 2022

adamcain-db Aug 2, 2022

wchau Aug 2, 2022

adamcain-db left a comment

pietern Aug 11, 2022

wchau Aug 11, 2022

pietern Aug 11, 2022

wchau Aug 11, 2022

pietern Aug 11, 2022

pietern Aug 11, 2022

wchau Aug 11, 2022

pietern Aug 11, 2022

wchau Aug 11, 2022

pietern left a comment

pietern Aug 12, 2022

		@@ -188,7 +197,7 @@ def delete_share_cli(api_client, name):
		@click.option('--allowed_ip_address', default=None, required=False, multiple=True,

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

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

Conversation

wchau commented Aug 2, 2022

codecov-commenter commented Aug 2, 2022 • edited Loading

Codecov Report

adamcain-db left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamcain-db left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pietern left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 2, 2022 •

edited

Loading