Add support for Libraries API#74
Conversation
mengxr
left a comment
mengxr
left a comment
There was a problem hiding this comment.
Made one pass. We should check the gist license issue and if uninstall requires maven-exclusion/.... Other comments are minor.
databricks_cli/click_types.py
Outdated
There was a problem hiding this comment.
- First sentence should describe the class.
- It doesn't have a license. We might need to remove the code if there is no default license for gist.
databricks_cli/libraries/cli.py
Outdated
There was a problem hiding this comment.
Don't quite understand why we need even if ....
databricks_cli/libraries/cli.py
Outdated
There was a problem hiding this comment.
need to update doc: Get the status of all libraries on all clusters.
databricks_cli/libraries/cli.py
Outdated
databricks_cli/libraries/cli.py
Outdated
There was a problem hiding this comment.
Not exactly an alias. Shall we say 'shortcut`?
| """ | ||
| if all: | ||
| library_statuses = _cluster_status(cluster_id).get('library_statuses', []) | ||
| libraries = [l_status['library'] for l_status in library_statuses] |
There was a problem hiding this comment.
Do we want to filter by LibraryInstallStatus? Curious what users see if this tries to uninstall a library that is in "FAILED" state.
There was a problem hiding this comment.
I don't think anything will happen, like the documentation says Uninstalling libraries that are not installed on the cluster will have no impact but is not an error.
databricks_cli/libraries/cli.py
Outdated
There was a problem hiding this comment.
This is duplicate code. Should do some refactoring.
databricks_cli/libraries/cli.py
Outdated
tests/libraries/test_cli.py
Outdated
tests/utils.py
Outdated
There was a problem hiding this comment.
- The method doesn't do what it claims. We can call it
assert_cli_outputand document the expected behavior. - It is better to combine assert and output_equals into a single method so you can output the actual and expected if they don't match. Currently, you only get a boolean output.
|
I've checked the I still have to check the gist license issue with Justin. |
| 'jar': 'dbfs:/test.jar', | ||
| } | ||
| }], | ||
| 'cluster_id': '0213-212348-veeps379' |
|
LGTM pending the gist license discussion |
andrewmchen
force-pushed
the
libraries
branch
from
4e6d6a7 to
439f477
Compare
2 participants
https://docs.databricks.com/api/latest/libraries.html