Skip to content

Commit

Permalink
Improve error handling by not failing group rename step if a group wa…
Browse files Browse the repository at this point in the history
…s removed from account before reflecting it to workspace (#762)

Fixes #761

In rare cases, it can happen that a group is removed from the account
before we manage to reflect it in the workspace. It's usually a couple
of minutes between listing the groups and reflecting it. This PR will
log such cases without failing the tool.

---------

Co-authored-by: Serge Smertin <259697+nfx@users.noreply.github.com>
  • Loading branch information
2 people authored and FastLee committed Jan 18, 2024
1 parent 9bebadd commit cc64764
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 1 deletion.
4 changes: 4 additions & 0 deletions src/databricks/labs/ucx/workspace_access/groups.py
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,10 @@ def _reflect_account_group_to_workspace(self, account_group_id: str):
except BadRequest:
# already exists
return True
except NotFound:
# the given group has been removed from the account after getting the group and before running this method
logger.warning("Group with ID: %s does not exist anymore in the Databricks account.", account_group_id)
return True

def _get_strategy(
self, workspace_groups_in_workspace: dict[str, Group], account_groups_in_account: dict[str, Group]
Expand Down
29 changes: 28 additions & 1 deletion tests/unit/workspace_access/test_groups.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import json
from unittest.mock import MagicMock

import pytest
from _pytest.outcomes import fail
from databricks.labs.blueprint.parallel import ManyError
from databricks.labs.blueprint.tui import MockPrompts
from databricks.sdk.errors import DatabricksError
from databricks.sdk.errors import DatabricksError, ResourceDoesNotExist
from databricks.sdk.service import iam
from databricks.sdk.service.iam import ComplexValue, Group, ResourceMeta

Expand Down Expand Up @@ -380,6 +381,32 @@ def do_side_effect(*args, **kwargs):
gm.reflect_account_groups_on_workspace()


def test_reflect_account_should_not_fail_if_group_not_in_the_account_anymore():
backend = MockBackend(rows={"SELECT": [("1", "de", "de", "test-group-de", "", "", "", "")]})
wsclient = MagicMock()
account_group1 = Group(id="11", display_name="de")

def reflect_account_side_effect(method, *args, **kwargs):
if method == "GET":
return {
"Resources": [g.as_dict() for g in [account_group1]],
}
if method == "PUT":
raise ResourceDoesNotExist(
"The group has been removed from the Databricks account after getting the group "
"and before reflecting it to the workspace."
)

wsclient.api_client.do.side_effect = reflect_account_side_effect
GroupManager(backend, wsclient, inventory_database="inv").reflect_account_groups_on_workspace()

wsclient.api_client.do.assert_called_with(
"PUT",
f"/api/2.0/preview/permissionassignments/principals/{account_group1.id}",
data=json.dumps({"permissions": ["USER"]}),
)


def test_delete_original_workspace_groups_should_delete_relected_acc_groups_in_workspace():
account_id = "11"
ws_id = "1"
Expand Down

0 comments on commit cc64764

Please sign in to comment.