Added `databricks labs ucx validate-groups-membership` command to validate groups to see if they have same membership across acount and workspace level by prajin-29 · Pull Request #772 · databrickslabs/ucx

prajin-29 · 2024-01-11T10:32:59Z

This pull request introduces several changes related to managing Databricks workspace groups. Here's a summary of the changes:

A new command validate-groups-membership is added to the CLI tool in the file cli.py. This command validates the groups in the workspace and account levels to see if they have different memberships. It returns a table showing the differences between the groups.
A new class GroupManager is added to the file groups.py to manage the groups in the workspace and account levels. This class includes a new method validate_group_membership that compares the members of each group in the workspace and account levels and returns a list of groups that have different memberships.
A new fixture make_ucx_group_with_diff_members is added to the file conftest.py to create a pair of groups with different members.
A new test case test_group_matching_names_with_diff_users is added to the file test_groups.py to test the validate_group_membership method with a pair of groups that have different members.
A retries decorator is added to the test case test_replace_workspace_groups_with_account_groups to handle potential intermittent failures.

These changes improve the group management functionality of the tool and make it more robust and reliable. The new validate-groups-membership command provides valuable information about the groups in the workspace and account levels, and the new GroupManager class and test cases ensure that the group management functionality works as expected.

Closes #649

nfx

do this functionality as a CLI command. You can rely on GroupManager, but don't rely on ucx.groups table - e.g. it should be possible to run databricks labs ucx validate-groups before running the assessment workflow

nfx · 2024-01-11T16:01:37Z

src/databricks/labs/ucx/workspace_access/groups.py

    entitlements: str | None = None
    external_id: str | None = None
    roles: str | None = None
+    is_same_membership: bool | None = None


no, you're not allowed to add this column, as it'll be a breaking change to database, which this small feature is not justifying

Changed the logic to incorporate it with CLI.

codecov · 2024-01-12T13:14:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (af80620) 84.07% compared to head (89f5e01) 84.19%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #772      +/-   ##
==========================================
+ Coverage   84.07%   84.19%   +0.12%     
==========================================
  Files          39       39              
  Lines        4872     4910      +38     
  Branches      913      919       +6     
==========================================
+ Hits         4096     4134      +38     
  Misses        564      564              
  Partials      212      212

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

prajin-29 · 2024-01-16T03:15:49Z

do this functionality as a CLI command. You can rely on GroupManager, but don't rely on ucx.groups table - e.g. it should be possible to run databricks labs ucx validate-groups before running the assessment workflow

@nfx .Did this in CLI Command. Can you please validate the same.

nfx · 2024-01-17T09:01:19Z

tests/integration/conftest.py



+@pytest.fixture
+def make_ucx_group_with_diff_members(make_random, make_group, make_acc_group, make_user):


Don't add a fixture if it's used only once. It makes the code convoluted and harder to maintain. Rewrite by inlining the fixture code into that single test

Removed this fixture and rewrote it inline with test.

nfx · 2024-01-17T09:03:47Z

tests/unit/workspace_access/test_groups.py

+
+def test_validate_group_diff_membership():
+    backend = MockBackend()
+    wsclient = MagicMock()


Use "create_autospec(WorkspaceClient)"

Used create_autospec

nfx · 2024-01-17T09:04:47Z

tests/unit/workspace_access/test_groups.py

+
+def test_validate_group_same_membership():
+    backend = MockBackend()
+    wsclient = MagicMock()


Create autospec

used create_autospec

william-conti

We should plan on enlarging this feature for all the workspaces in the account

william-conti · 2024-01-18T08:42:53Z

src/databricks/labs/ucx/cli.py

+        account_group_regex=account_group_regex,
+    )
+    mismatch_groups = group_manager.validate_group_membership()
+    print(json.dumps(mismatch_groups))


It should be better to return an error code when there's at least one entry in the mismatch_groups

I'm not sure if CLI wrapper supports that just yet

william-conti · 2024-01-18T08:46:52Z

src/databricks/labs/ucx/cli.py



+@ucx.command
+def validate_groups_membership(w: WorkspaceClient):


What if I wan't to scope it for all workspaces in my account ?

It has to be an additional command with is_account_level=True, it should call this function and pass down a workspace client for each workspace

nfx

Inline fixture that is not reused
Make variable names consistent
Add additional account level command

nfx · 2024-01-18T09:31:10Z

labs.yml

+  - name: validate-groups-membership
+    description: Validate the groups to see if the groups at account level and workspace level have different membership
+    table_template: |-
+      Workflow_Group_Name\tAccount_Group_Name


Use spaces for separating words, not underscores

Replaced _ with space

nfx · 2024-01-18T09:33:19Z

labs.yml

+    description: Validate the groups to see if the groups at account level and workspace level have different membership
+    table_template: |-
+      Workflow_Group_Name\tAccount_Group_Name
+      {{range .}}{{.wf_group_name}}\t{{.ac_group_name}}


Suggested change

{{range .}}{{.wf_group_name}}\t{{.ac_group_name}}

{{range .}}{{.name_in_workspace}}\t{{.name_in_account}}

Use conventional variable names

Renamed with the conventional names

nfx · 2024-01-18T09:34:52Z

src/databricks/labs/ucx/cli.py

+        account_group_regex=account_group_regex,
+    )
+    mismatch_groups = group_manager.validate_group_membership()
+    print(json.dumps(mismatch_groups))


I'm not sure if CLI wrapper supports that just yet

# Conflicts: # labs.yml # tests/unit/test_cli.py

* Added `databricks labs ucx validate-groups-membership` command to validate groups to see if they have same membership across acount and workspace level ([#772](#772)). * Added baseline for getting Azure Resource Role Assignments ([#764](#764)). * Added issue and pull request templates ([#791](#791)). * Added linked issues to PR template ([#793](#793)). * Added optional `debug_truncate_bytes` parameter to the config and extend the default log truncation limit ([#782](#782)). * Added support for crawling grants and applying Hive Metastore UDF ACLs ([#812](#812)). * Changed Python requirement from 3.10.6 to 3.10 ([#805](#805)). * Extend error handling of delta issues in crawlers and hive metastore ([#795](#795)). * Fixed `databricks labs ucx repair-run` command to execute correctly ([#801](#801)). * Fixed handling of `DELTASHARING` table format ([#802](#802)). * Fixed listing of workflows via CLI ([#811](#811)). * Fixed logger import path for DEBUG notebook ([#792](#792)). * Fixed move table command to delete table/view regardless if permissions are present, skipping corrupted tables when crawling table size and making existing tests more stable ([#777](#777)). * Fixed the issue of `databricks labs ucx installations` and `databricks labs ucx manual-workspace-info` ([#814](#814)). * Increase the unit test coverage for cli.py ([#800](#800)). * Mount Point crawler lists /Volume with four variations which is confusing ([#779](#779)). * Updated README.md to remove mention of deprecated install.sh ([#781](#781)). * Updated `bug` issue template ([#797](#797)). * Fixed writing log readme in multiprocess safe way ([#794](#794)).

prajin-29 added 2 commits January 11, 2024 15:55

Flagging Groups with different membership

96de166

Flagging Groups with different membership

d1edf5f

prajin-29 requested review from a team and pritishpai January 11, 2024 10:33

prajin-29 had a problem deploying to account-admin January 11, 2024 10:33 — with GitHub Actions Failure

nfx requested changes Jan 11, 2024

View reviewed changes

prajin-29 added 3 commits January 12, 2024 13:06

Updating to ClI

82d2470

Updating to ClI

2c50cc8

Merge branch 'main' into feature/log_group_membership

bd444ee

prajin-29 temporarily deployed to account-admin January 12, 2024 13:13 — with GitHub Actions Inactive

prajin-29 changed the title ~~Logging Groups that does not have same membrship at account and workspace level~~ Added databricks labs ucx validate-groups-membership command to validate groups to see if they have same membership across acount and workspace level Jan 12, 2024

prajin-29 added 3 commits January 14, 2024 18:06

Updating test case for cli

21b909f

Merge branch 'main' into feature/log_group_membership

9bfd017

Updating test case and integration testing

2407e3c

prajin-29 temporarily deployed to account-admin January 14, 2024 13:30 — with GitHub Actions Inactive

Updating test case and integration testing

a148109

prajin-29 had a problem deploying to account-admin January 14, 2024 13:34 — with GitHub Actions Failure

Updated linting

48f38a4

prajin-29 temporarily deployed to account-admin January 14, 2024 13:44 — with GitHub Actions Inactive

nfx requested changes Jan 17, 2024

View reviewed changes

william-conti suggested changes Jan 18, 2024

View reviewed changes

nfx requested changes Jan 18, 2024

View reviewed changes

prajin-29 added 2 commits January 18, 2024 21:10

Merge branch 'main' into feature/log_group_membership

f90beb2

# Conflicts: # labs.yml # tests/unit/test_cli.py

Updated the Comments for the PR

89f5e01

prajin-29 had a problem deploying to account-admin January 18, 2024 16:32 — with GitHub Actions Failure

nfx approved these changes Jan 19, 2024

View reviewed changes

prajin-29 had a problem deploying to account-admin January 19, 2024 09:17 — with GitHub Actions Failure

prajin-29 temporarily deployed to account-admin January 19, 2024 09:38 — with GitHub Actions Inactive

nfx merged commit 0948141 into main Jan 19, 2024

nfx deleted the feature/log_group_membership branch January 19, 2024 09:54

nfx mentioned this pull request Jan 19, 2024

Release v0.10.0 #819

Merged

nfx mentioned this pull request Mar 26, 2024

Notify account/scim administrators about workspace groups not present as Databricks Account group #342

Closed



		@pytest.fixture
		def make_ucx_group_with_diff_members(make_random, make_group, make_acc_group, make_user):



		@ucx.command
		def validate_groups_membership(w: WorkspaceClient):

	{{range .}}{{.wf_group_name}}\t{{.ac_group_name}}
	{{range .}}{{.name_in_workspace}}\t{{.name_in_account}}

Conversation

prajin-29 commented Jan 11, 2024 • edited by nfx Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nfx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

prajin-29 commented Jan 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

william-conti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nfx Jan 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nfx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

prajin-29 commented Jan 11, 2024 •

edited by nfx

Loading

codecov bot commented Jan 12, 2024 •

edited

Loading

nfx Jan 18, 2024 •

edited

Loading