Skip to content

Conversation

@wchau
Copy link
Contributor

@wchau wchau commented Aug 11, 2022

Sharing code is not used anymore.

Tested locally.

@wchau wchau force-pushed the ds_create_recipient_id branch from 81de72b to 1119681 Compare August 11, 2022 21:01
@wchau wchau assigned wchau and unassigned wchau Aug 11, 2022
@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2022

Codecov Report

Merging #534 (de33148) into main (93a8fe2) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##             main     #534   +/-   ##
=======================================
  Coverage   60.72%   60.72%           
=======================================
  Files          55       55           
  Lines        4692     4692           
=======================================
  Hits         2849     2849           
  Misses       1843     1843           
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%> (ø)

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

@wchau
Copy link
Contributor Author

wchau commented Aug 11, 2022

Do we still need to fix public API even if sharing code is useless today?

@wchau wchau requested review from adamcain-db and pietern August 11, 2022 21:07
Copy link
Contributor

@adamcain-db adamcain-db left a comment

Choose a reason for hiding this comment

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

The databricks-cli changes LGTM, though I know nothing of the underlying CreateRecipient API change. Should a delta-sharing person also review?

@wchau wchau requested a review from zhuansunxt August 11, 2022 21:25
Copy link
Contributor

@zhuansunxt zhuansunxt left a comment

Choose a reason for hiding this comment

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

In the UI we use the term sharing identifier. I think the rationale behind that is we want to be more abstract so we can change the format later. Let's keep it consistent in the CLI. (probably too late to migrate REST API)

Copy link
Contributor

@zhuansunxt zhuansunxt left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

We can ignore the API compatibility test as these functions are unlikely to already be in use.

@pietern pietern merged commit ef918f1 into databricks:main Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants