Skip to content
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

Improve error handling by not failing group rename step if a group was removed from account before reflecting it to workspace #762

Merged
merged 6 commits into from
Jan 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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()
mwojtyczka marked this conversation as resolved.
Show resolved Hide resolved

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
Loading