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.

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

Add 'metastores get-assignment' CLI for UC #527

Merged

Conversation

adamcain-db
Copy link
Contributor

  • Adds unity-catalog metastores get-assignment (a.k.a. unity-catalog get-metastore-assignment) for exercising new UC getCurrentMetastoreAssignment API endpoint

@codecov-commenter
Copy link

codecov-commenter commented Jul 31, 2022

Codecov Report

Merging #527 (bb03c1f) into main (44ccc7d) will decrease coverage by 0.19%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #527      +/-   ##
==========================================
- Coverage   60.93%   60.73%   -0.20%     
==========================================
  Files          55       55              
  Lines        4669     4684      +15     
==========================================
  Hits         2845     2845              
- Misses       1824     1839      +15     
Impacted Files Coverage Δ
databricks_cli/unity_catalog/api.py 0.00% <0.00%> (ø)
databricks_cli/unity_catalog/metastore_cli.py 0.00% <0.00%> (ø)
databricks_cli/unity_catalog/uc_service.py 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44ccc7d...bb03c1f. Read the comment docs.

@adamcain-db adamcain-db requested a review from wchau August 1, 2022 21:33
@@ -193,6 +206,8 @@ def register_metastore_commands(cmd_group):
cmd_group.add_command(hide(metastore_summary_cli), name='metastore-summary')
cmd_group.add_command(hide(assign_metastore_cli), name='assign-metastore')
cmd_group.add_command(hide(unassign_metastore_cli), name='unassign-metastore')
cmd_group.add_command(hide(get_metastore_assignment_cli),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this? Based on the comment, these are "deprecated" commands

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I just kinda liked the get-metastore-assignment version. Can remove it, if you prefer.

@adamcain-db adamcain-db requested a review from pietern August 2, 2022 00:24
@adamcain-db
Copy link
Contributor Author

I'll wait for @pietern to have a chance to review before I merge this.

@adamcain-db adamcain-db requested a review from fjakobs August 9, 2022 06:04
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.

LGTM

@adamcain-db Can you add one or two UC tests to set a precedent? Can be done as separate PR. Would be good to ensure new functionality gets tested. For this one it would be good to have a mocked response payload and expectations on the output. It looks like the current approach just dumps the JSON returned by the server, but it might be more in line with expectations to only print the metastore ID.

@pietern
Copy link
Contributor

pietern commented Aug 10, 2022

@adamcain-db adamcain-db force-pushed the add-get-current-metastore-assignment branch from 620d175 to 663ad19 Compare August 11, 2022 01:46
@adamcain-db adamcain-db force-pushed the add-get-current-metastore-assignment branch from 663ad19 to 042cb00 Compare August 11, 2022 02:02
@adamcain-db
Copy link
Contributor Author

@pietern:

It looks like the current approach just dumps the JSON returned by the server, but it might be more in line with expectations to only print the metastore ID.

I think we should keep dumping the full JSON (as done with the other unity-catalog commands). Turns out, the default_catalog_name part of the assignment is of major interest to some users (e.g., the Partner Connect folks).

I'll add unit tests in a separate PR.

@pietern
Copy link
Contributor

pietern commented Aug 11, 2022

Understood, thanks for clarifying.

@pietern pietern merged commit 93a8fe2 into databricks:main Aug 11, 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.

4 participants