WIP: Add databricks permissions command #314

areese · 2020-08-17T20:28:35Z

This is a public preview api

gauthamsunjay

Thanks for the PR. Left some comments.

gauthamsunjay · 2020-08-20T23:20:58Z

databricks_cli/permissions/api.py

+        }
+
+
+class PermissionsApi(object):


This class doesn't expose a way to set / update permissions.

Thank you, that's a good point I'll fix it.

I'll add update, but looking at https://docs.databricks.com/dev-tools/api/latest/permissions.html#operation/update-all-notebook-permissions

I'm kinda skeptical of set_permissions. The issue I have is it's dangerous, you could wipe all permission out everywhere if you are not careful.

I'm going to leave that out of this pr, and start a thread with engineering to determine if that's something that should be exposed.

databricks_cli/permissions/api.py

databricks_cli/permissions/cli.py

gauthamsunjay · 2020-08-21T00:01:57Z

databricks_cli/permissions/cli.py

+        return
+
+    click.echo(pretty_format(perms_api.add_permissions(object_type, object_id, all_permissions)))
+


We should add support for the following:

Set / update permissions

Ability to specify multiple permissions (not sure if this is possible already)

I'll take note of that.

I also need to add some tests, and I'll cover those as part of the tests.

gauthamsunjay · 2020-08-21T00:02:25Z

tests/permissions/test_cli.py

+    }
+}
+
+


Can we add tests for other operations as well?

Yes. :). Oversight.

I'm trying to collect all of the bits I have, and get them pushed.
Once there are tests and better coverage, we can take another look.

Move get_id_for_directory from PermissionsApi to WorkspaceApi

…n error that said level. :(

…nts, as that breaks existing code

…ace api

areese · 2020-09-18T17:32:08Z

This is still WIP, there are a number of requests from @gauthamsunjay that need to be addressed, once those are fixed, we can review again.

…ssions api calls to make life easier

Move from argument to option so help can be provided. Share the same help text across all of the databricks permissions functions

Move from --object_type to --object-id to be inline with other commands Fix tests to validate the test output the same way clusters does

…ype hint not being the first line of the functions Remove an unused variable from tests/permissions/test_cli.py

1. Reuse the same help string for all object types 2. Update the group help to display the object types, it could be hard coded or displayed a lot cleaner. 3. object-id is always required, even on list, need to find out why 4. Remove if not object_type checks, as object type is a required option 5. Add mostly complete tests for list-permission-types, I do not have registered-models or instance-pools data available

Fix line too long lint errors. Fix errors looking up instance-pools in the enum

$ databricks permissions add --object-type clusters --object-id foo Error: Missing None. One of ['group-name', 'user-name', 'service-name'] must be provided. $ databricks permissions add --object-type clusters --object-id foo Error: Missing option '--group-name'. One of ['group-name', 'user-name', 'service-name'] must be provided.

Cleanup ordering of commands added to the group Remove checks that are done by click and are not required in databricks permissions add

Rename databricks_cli/permissions/api.py:Lookups to PermissionsLookup PermissionsLookup.from_name should raise a useful error with a message instead of KeyError Remove more error checking that is done by click. --permission-level should be checked by click to provide a useful error

Use click.Choice to validate permission levels Add test for databricks permissions add clusters manage

…ions api and service better

Allow PermissionsObject to init with constructor databricks permissions ls should require path, and path should have help Add more tests

Remove dead code that wasn't called Update tests to cover all of the commands

…detecting bad combinations easier

nfx

please see comments

nfx · 2020-10-20T09:20:24Z

databricks_cli/permissions/api.py

+
+
+class PermissionTargets(Enum):
+    clusters = 'clusters'


logically this should rather be a dictionary or list of tuples, because it's mixing two different things under same enumeration.

e.g. cluster is the object type and clusters is the prefix for object it,

nfx · 2020-10-20T09:25:47Z

databricks_cli/permissions/api.py

+        return ', '.join([e.value for e in PermissionLevel])
+
+
+class BasicPermissions(object):


it looks over-engineered. please simplify.

technically, it all can be replaced by single simple lookup structure.

nfx · 2020-10-20T09:31:26Z

databricks_cli/permissions/cli.py

+@click.command(context_settings=CONTEXT_SETTINGS,
+               short_help='Get permissions for an item.  ' + POSSIBLE_OBJECT_TYPES)
+@click.option('--object-type', required=True, help=POSSIBLE_OBJECT_TYPES)
+@click.option('--object-id', required=True, help='object id to require permission about')


why not providing one of kind of arguments? e.g. --cluster-id, --job-id, --tokens, --passwords, --cluster-policy-id and etc? CLI is supposed to simplify usage for admins and not always be 1-1 mapping to API resources.

nfx · 2020-10-20T09:31:54Z

databricks_cli/permissions/cli.py

+@click.option('--group-name', metavar='<string>', cls=OneOfOption,
+              one_of=GROUP_USER_SERVICE_OPTIONS)
+@click.option('--user-name', metavar='<string>', cls=OneOfOption, one_of=GROUP_USER_SERVICE_OPTIONS)
+@click.option('--service-name', metavar='<string>', cls=OneOfOption,


it's not really a service name, but --service-principal

nfx · 2020-10-20T09:33:00Z

databricks_cli/permissions/exceptions.py

+# limitations under the License.
+
+
+class PermissionsError(Exception):


why do we need a file for two lines of code? why it cannot be in the same file as where it's thrown from?

nfx · 2020-10-20T09:34:34Z

databricks_cli/sdk/permissions_service.py

@@ -0,0 +1,95 @@
+from typing import Optional
+
+from databricks_cli.sdk.preview_service import PreviewService


there's no such a thing as preview service. please remove or rename to something like PreviewStability

That's reasonable.

I'm probably going to use the newer openapi spec that is part of the rest specification and rewrite this differently.

areese · 2021-01-06T17:08:19Z

FYI: This is stale, it has semi-useful code but has a lot to be addressed.
I'm going to leave it open, but I'm not sure how fast I'll come back to it.

Allen Reese added 4 commits August 17, 2020 13:27

Add databricks permissions command

2088c40

This is a public preview api

Remove extra file

407abe7

Fix acidental removal of tests/__init__.py

631f20b

tests/permissions/__init__.py was missing the license

c5075e1

gauthamsunjay reviewed Aug 21, 2020

View reviewed changes

Allen Reese added 6 commits September 18, 2020 09:32

Merge branch 'master' into add-permissions

18daab4

WIP: Add missing bits in the workspace API

6f08afd

Move get_id_for_directory from PermissionsApi to WorkspaceApi

Fix an issue where the if statements were checking type but raising a…

4e17f06

…n error that said level. :(

Remove WorkspaceApi import

123a805

Fix a typo in WorkspaceFileInfo. We cannot change the order of argume…

c00458b

…nts, as that breaks existing code

get_id_for_directory was moved from the permissions api to the worksp…

4b7302c

…ace api

areese changed the title ~~Add databricks permissions command~~ WIP: Add databricks permissions command Sep 18, 2020

Allen Reese added 17 commits September 22, 2020 09:47

Fix lint errors about un-grouped imports

e10f6f6

Add some documentation showing all of the various urls that the permi…

e8d55c0

…ssions api calls to make life easier

databricks permissions:

849944c

Move from argument to option so help can be provided. Share the same help text across all of the databricks permissions functions

databricks cli permissions

5b59855

Move from --object_type to --object-id to be inline with other commands Fix tests to validate the test output the same way clusters does

Fix a lint error in databricks_cli/sdk/permissions.py caused by the t…

12da975

…ype hint not being the first line of the functions Remove an unused variable from tests/permissions/test_cli.py

databricks permssions

eef1210

Fix line too long lint errors. Fix errors looking up instance-pools in the enum

databricks permssions

d49cd78

Cleanup ordering of commands added to the group Remove checks that are done by click and are not required in databricks permissions add

databricks permissions

cccf174

Use click.Choice to validate permission levels Add test for databricks permissions add clusters manage

Add more tests of the permissions cli, still need to test the permiss…

25e710f

…ions api and service better

Fix lint errors

6ca0a00

Merge remote-tracking branch 'upstream/master' into add-permissions

cfe232e

databricks permissions

d9ba966

Allow PermissionsObject to init with constructor databricks permissions ls should require path, and path should have help Add more tests

databricks permissions:

fb5f354

Remove dead code that wasn't called Update tests to cover all of the commands

Checkpoint trying to cleanup how the permissions are managed to make …

83c6688

…detecting bad combinations easier

Merge branch 'master' into add-permissions

b275af6

nfx requested changes Oct 20, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add databricks permissions command #314

WIP: Add databricks permissions command #314

areese commented Aug 17, 2020

gauthamsunjay left a comment

gauthamsunjay Aug 20, 2020

areese Aug 26, 2020

areese Sep 18, 2020

gauthamsunjay Aug 21, 2020

areese Aug 26, 2020

gauthamsunjay Aug 21, 2020

areese Aug 26, 2020

areese commented Sep 18, 2020

nfx left a comment

nfx Oct 20, 2020

nfx Oct 20, 2020

nfx Oct 20, 2020

nfx Oct 20, 2020

nfx Oct 20, 2020

nfx Oct 20, 2020

areese Oct 20, 2020

areese commented Jan 6, 2021

		return

		click.echo(pretty_format(perms_api.add_permissions(object_type, object_id, all_permissions)))

		return ', '.join([e.value for e in PermissionLevel])


		class BasicPermissions(object):

		# limitations under the License.


		class PermissionsError(Exception):

		@@ -0,0 +1,95 @@
		from typing import Optional

		from databricks_cli.sdk.preview_service import PreviewService

WIP: Add databricks permissions command #314

Are you sure you want to change the base?

WIP: Add databricks permissions command #314

Conversation

areese commented Aug 17, 2020

gauthamsunjay 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

areese commented Sep 18, 2020

nfx 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

areese commented Jan 6, 2021