-
Notifications
You must be signed in to change notification settings - Fork 234
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
Framework for creating and using the Unity Catalog connections API #647
Conversation
|
||
# These two options are shared among create and updates, so they are very common | ||
def create_update_common_options(f): | ||
@click.option('--read-only/--no-read-only', is_flag=True, default=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: 'Whether the connection is read-only'
… get, list, and delete commands as well
@eat_exceptions | ||
@profile_option | ||
@provide_api_client | ||
def create_online_catalog_cli(api_client, name, host, port, user, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I forgot to say this is not user creatable! Let's remove this.
Create new connection with an inline JSON or JSON file input. | ||
''' | ||
if (json is None) and (json_file is None): | ||
raise ValueError('Must either provide inline JSON or JSON file.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "Must either provide inline JSON or JSON file path."
@profile_option | ||
@eat_exceptions | ||
@provide_api_client | ||
def list_connections_cli(api_client, ): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need the extra comma?
@profile_option | ||
@eat_exceptions | ||
@provide_api_client | ||
def delete_connection_cli(api_client, name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we omit the update_connection_cli?
tests/unity_catalog/test_con_cli.py
Outdated
COMMENT = 'some_comment' | ||
|
||
TESTHOST = "test_postgresql.fakedb.com" | ||
TESTHOST2 = "postgresql.fakedb2.lan" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TESTHOST2 and TESTPORT2 are unused?
|
||
|
||
@provide_conf | ||
def test_create_connection_cli(api_mock): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add tests for get, update, delete list as well?
@jamesd-db Good news - @pietern is working on a new Databricks CLI that automatically generates client code based on open api spec - so we will have coverage automatically. Looks like it's already showing on the SDK: https://github.com/databricks/databricks-sdk-go/blob/1504930d8c20337f353f13d4934700d85e0b6ed5/service/catalog/interface.go#L177-L220 We can stop polishing this PR or adding tests and just use this as a test environment for our OAuth work! |
472e043
to
4f34d99
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## uc-connections #647 +/- ##
==================================================
- Coverage 76.56% 75.44% -1.13%
==================================================
Files 55 56 +1
Lines 5048 5387 +339
==================================================
+ Hits 3865 4064 +199
- Misses 1183 1323 +140
☔ View full report in Codecov by Sentry. |
I think that it's better to switch to the new Databricks CLI and Python SDKs that are automatically generated from OpenAPI specs |
@alexott we chatted with @pietern - we just want to archive this code (it is mainly used for testing oauth support for query federation connections) in its dedicated branch since it was worked on by our intern before the new cli was live and also we might not have time to convert it into the new cli during the internship. We eventually will move this to the new cli but likely after @jamesd-db's internship ends. So @pietern created this branch for us. |
By merging we archive the work on a branch for future reference. No intention of merging this into main. |
@andrewli81 Is this PR otherwise done? |
@pietern Yes this PR is otherwise done, @jamesd-db uploaded the OAuth client changes and pointed this PR to the branch you created, we should be good to merge. |
References to JIRA: SC-133066 and SC-134677 |
Creating a framework for creating and using the Unity Catalog connections API with the Databricks CLI.