Remove redundant internal methods from create_account_group (#3395)

<!-- REMOVE IRRELEVANT COMMENTS BEFORE CREATING A PULL REQUEST -->
## Changes
changed the signature of create_account_groups and removed the
workspace_id parameter and instead retrieve it from
accountworkspace._workspaces() which contains the account context
workspace_id named parameter
<!-- Summary of your changes that are easy to understand. Add
screenshots when necessary -->


Resolves #3170 

### Functionality

- [ ] modified existing command: `databricks labs ucx ...`

### Tests
<!-- How is this tested? Please see the checklist below and also
describe any other relevant tests -->

- [ ] added unit tests

Author: HariGS-DB

src/databricks/labs/ucx/account/workspaces.py

@@ -76,9 +76,11 @@ def sync_workspace_info(self, workspaces: list[Workspace] | None = None):
             except (PermissionDenied, NotFound, ValueError):
                 logger.warning(f"Failed to save workspace info for {ws.config.host}")
 
-    def create_account_level_groups(self, prompts: Prompts, workspace_ids: list[int] | None = None):
+    def create_account_level_groups(self, prompts: Prompts):
         acc_groups = self._get_account_groups()
-        workspace_ids = self._get_valid_workspaces_ids(workspace_ids)
+        workspace_ids = [workspace.workspace_id for workspace in self._workspaces()]
+        if not workspace_ids:
+            raise ValueError("The workspace ids provided are not found in the account, Please check and try again.")
         all_valid_workspace_groups = self._get_valid_workspaces_groups(prompts, workspace_ids)
 
         for group_name, valid_group in all_valid_workspace_groups.items():
@@ -136,26 +138,6 @@ def _try_create_account_groups(
             logger.info(f"Group {group_name} already exist in the account, ignoring")
             return None
 
-    def _get_valid_workspaces_ids(self, workspace_ids: list[int] | None = None) -> list[int]:
-        all_workspace_ids = [workspace.workspace_id for workspace in self._workspaces()]
-        if not workspace_ids:
-            return all_workspace_ids
-        # TODO: remove this method and rely on _include_workspace_ids
-
-        valid_workspace_ids = []
-        for workspace_id in workspace_ids:
-            if workspace_id in all_workspace_ids:
-                valid_workspace_ids.append(workspace_id)
-            else:
-                logger.info(f"Workspace id {workspace_id} not found on the account")
-
-        if not valid_workspace_ids:
-            raise ValueError("No workspace ids provided in the configuration found in the account")
-
-        workspace_ids_str = ','.join(str(x) for x in valid_workspace_ids)
-        logger.info(f"Creating account groups for workspaces IDs : {workspace_ids_str}")
-        return valid_workspace_ids
-
     def _add_members_to_acc_group(
         self, acc_client: AccountClient, acc_group_id: str, group_name: str, valid_group: Group
     ):

src/databricks/labs/ucx/cli.py

@@ -170,7 +170,7 @@ def create_account_groups(
     """
     if not ctx:
         ctx = AccountContext(a, named_parameters)
-    ctx.account_workspaces.create_account_level_groups(prompts, ctx.workspace_ids)
+    ctx.account_workspaces.create_account_level_groups(prompts)
 
 
 @ucx.command

tests/integration/account/test_account.py

@@ -38,7 +38,7 @@ def test_create_account_level_groups(
 
     group_display_name = f"created_by_ucx_regular_group-{suffix}"
     make_group(display_name=group_display_name, members=[make_user().id])
-    AccountWorkspaces(acc).create_account_level_groups(MockPrompts({}), [ws.get_workspace_id()])
+    AccountWorkspaces(acc, [ws.get_workspace_id()]).create_account_level_groups(MockPrompts({}))
 
     @retried(on=[KeyError], timeout=timedelta(minutes=2))
     def get_group(display_name: str) -> Group:

tests/unit/account/test_workspaces.py

@@ -81,8 +81,8 @@ def test_create_acc_groups_should_create_acc_group_if_no_group_found_in_account(
     acc_client.get_workspace_client.return_value = ws
     acc_client.groups.create.return_value = group
 
-    account_workspaces = AccountWorkspaces(acc_client)
-    account_workspaces.create_account_level_groups(MockPrompts({}), [123, 46])
+    account_workspaces = AccountWorkspaces(acc_client, [123, 46])
+    account_workspaces.create_account_level_groups(MockPrompts({}))
 
     acc_client.groups.create.assert_called_with(
         display_name="de",
@@ -109,9 +109,9 @@ def test_create_acc_groups_should_throw_exception(acc_client):
     acc_client.get_workspace_client.return_value = ws
     acc_client.groups.create.return_value = group
 
-    account_workspaces = AccountWorkspaces(acc_client)
+    account_workspaces = AccountWorkspaces(acc_client, [123])
     with pytest.raises(ValueError):
-        account_workspaces.create_account_level_groups(MockPrompts({}), [123])
+        account_workspaces.create_account_level_groups(MockPrompts({}))
 
     ws.groups.list.assert_not_called()
 
@@ -133,8 +133,8 @@ def test_create_acc_groups_should_filter_system_groups(acc_client):
     acc_client.get_workspace_client.return_value = ws
     acc_client.groups.create.return_value = group
 
-    account_workspaces = AccountWorkspaces(acc_client)
-    account_workspaces.create_account_level_groups(MockPrompts({}), [123])
+    account_workspaces = AccountWorkspaces(acc_client, [123])
+    account_workspaces.create_account_level_groups(MockPrompts({}))
 
     acc_client.groups.create.assert_not_called()
 
@@ -164,7 +164,7 @@ def test_create_acc_groups_should_create_acc_group_with_appropriate_members(acc_
     ]
 
     ws = create_autospec(WorkspaceClient)
-    account_workspaces = AccountWorkspaces(acc_client)
+    account_workspaces = AccountWorkspaces(acc_client, [123])
 
     group = Group(
         id="12",
@@ -203,7 +203,7 @@ def test_create_acc_groups_should_create_acc_group_with_appropriate_members(acc_
     acc_client.get_workspace_client.return_value = ws
     acc_client.groups.create.return_value = group
 
-    account_workspaces.create_account_level_groups(MockPrompts({}), [123])
+    account_workspaces.create_account_level_groups(MockPrompts({}))
 
     acc_client.groups.create.assert_called_with(
         display_name="de",
@@ -276,8 +276,8 @@ def test_create_acc_groups_should_not_create_group_if_exists_in_account(acc_clie
     ws.groups.list.return_value = [group]
     ws.groups.get.return_value = group
     acc_client.get_workspace_client.return_value = ws
-    account_workspaces = AccountWorkspaces(acc_client)
-    account_workspaces.create_account_level_groups(MockPrompts({}), [123])
+    account_workspaces = AccountWorkspaces(acc_client, [123])
+    account_workspaces.create_account_level_groups(MockPrompts({}))
 
     acc_client.groups.create.assert_not_called()
 
@@ -307,8 +307,8 @@ def get_workspace_client(workspace) -> WorkspaceClient:
 
     acc_client.get_workspace_client.side_effect = get_workspace_client
 
-    account_workspaces = AccountWorkspaces(acc_client)
-    account_workspaces.create_account_level_groups(MockPrompts({}), [123, 456])
+    account_workspaces = AccountWorkspaces(acc_client, [123, 456])
+    account_workspaces.create_account_level_groups(MockPrompts({}))
 
     acc_client.groups.create.assert_any_call(display_name="de")
     acc_client.groups.create.assert_any_call(display_name="security_grp")
@@ -342,8 +342,8 @@ def get_workspace_client(workspace) -> WorkspaceClient:
     acc_client.groups.create.return_value = group
     acc_client.get_workspace_client.side_effect = get_workspace_client
 
-    account_workspaces = AccountWorkspaces(acc_client)
-    account_workspaces.create_account_level_groups(MockPrompts({}), [123, 456])
+    account_workspaces = AccountWorkspaces(acc_client, [123, 456])
+    account_workspaces.create_account_level_groups(MockPrompts({}))
 
     acc_client.groups.create.assert_called_once_with(display_name="de")
     acc_client.groups.patch.assert_called_once_with(
@@ -391,14 +391,13 @@ def get_workspace_client(workspace) -> WorkspaceClient:
     ws2.groups.get.return_value = group_2
     acc_client.get_workspace_client.side_effect = get_workspace_client
 
-    account_workspaces = AccountWorkspaces(acc_client)
+    account_workspaces = AccountWorkspaces(acc_client, [123, 456])
     account_workspaces.create_account_level_groups(
         MockPrompts(
             {
                 r'Group de does not have the same amount of members in workspace ': 'yes',
             }
-        ),
-        [123, 456],
+        )
     )
 
     acc_client.groups.create.assert_any_call(display_name="de")
@@ -423,8 +422,8 @@ def test_acc_ws_get_should_not_throw(acc_client):
     ws.groups.list.return_value = [group]
     ws.groups.get.side_effect = NotFound
     acc_client.get_workspace_client.return_value = ws
-    account_workspaces = AccountWorkspaces(acc_client)
-    account_workspaces.create_account_level_groups(MockPrompts({}), [123])
+    account_workspaces = AccountWorkspaces(acc_client, [123])
+    account_workspaces.create_account_level_groups(MockPrompts({}))
 
     acc_client.groups.create.assert_not_called()
 
@@ -435,7 +434,7 @@ def test_create_acc_groups_should_not_throw_if_acc_grp_exists(acc_client):
     ]
 
     ws = create_autospec(WorkspaceClient)
-    account_workspaces = AccountWorkspaces(acc_client)
+    account_workspaces = AccountWorkspaces(acc_client, [123])
 
     group = Group(id="12", display_name="de", members=[ComplexValue(display="test-user-1", value="1")])
 
@@ -444,7 +443,7 @@ def test_create_acc_groups_should_not_throw_if_acc_grp_exists(acc_client):
     acc_client.get_workspace_client.return_value = ws
     acc_client.groups.create.side_effect = ResourceConflict
 
-    account_workspaces.create_account_level_groups(MockPrompts({}), [123])
+    account_workspaces.create_account_level_groups(MockPrompts({}))
 
     acc_client.groups.create.assert_called_with(display_name="de")
     acc_client.groups.patch.assert_not_called()

tests/unit/test_cli.py

@@ -339,9 +339,10 @@ def test_create_account_groups():
     a = create_autospec(AccountClient)
     w = create_autospec(WorkspaceClient)
     a.get_workspace_client.return_value = w
+    a.workspaces.list.return_value = [Workspace(workspace_id=123)]
     w.get_workspace_id.return_value = None
     prompts = MockPrompts({})
-    ctx = AccountContext(a).replace()
+    ctx = AccountContext(a, {"workspace_ids": "123,456"})
     create_account_groups(a, prompts, ctx=ctx)
     a.groups.list.assert_called_with(attributes="id")
 
@@ -353,7 +354,9 @@ def test_create_account_groups_with_id():
     w.get_workspace_id.return_value = None
     prompts = MockPrompts({})
     ctx = AccountContext(a, {"workspace_ids": "123,456"})
-    with pytest.raises(ValueError, match="No workspace ids provided in the configuration found in the account"):
+    with pytest.raises(
+        ValueError, match="The workspace ids provided are not found in the account, Please check and try again."
+    ):
         create_account_groups(a, prompts, ctx=ctx)