Merge ~rgildein/juju-verify:bug1921505 into ~canonical-solutions-engineering/juju-verify:master

[Robert Gildein]

removed `BaseVerifier.run_action_on_units`

The function `BaseVerifier.run_action_on_units` was used with restriction
that prevents actions being run on units other than those that are part
of the self.units.

Because this restriction was no longer useful, it has been removed, and
without it, there is no reason for the `run_action_on_units` function
to be part of the `BaseVerifier`. At the same time, the
`run_action_on_unit` function has been moved from `BaseVerifier` to
`utils.unit`.

Fixes: LP#1921505

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:600312f24dd930e40f5166815bfec2463de250e9
https://jenkins.canonical.com/bootstack/job/lp-juju-verify-ci/38/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-full/30/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/160/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-juju-verify-ci/38//rebuild

review: Approve (continuous-integration)

[Alvaro Uria (aluria)]

+1, change lgtm. Have you run the functional tests to confirm all looks good?

review: Needs Information

[Martin Kalcok (martin-kalcok)]

I ran the func tests and they all passed.

Change LGTM, thanks.

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision f5f65933228c96286b4edb8469672b7cba0916dd

diff --git a/juju_verify/utils/unit.py b/juju_verify/utils/unit.py
index 84bc3f8..6ff3fb0 100644
--- a/juju_verify/utils/unit.py
+++ b/juju_verify/utils/unit.py
@@ -66,6 +66,17 @@ def run_action_on_units(
     return result_map
 
 
+def run_action_on_unit(unit: Unit, action: str, **params: str) -> Action:
+    """Run juju action on single unit.
+
+    For more info, see docstring for 'run_action_on_units'. The only
+    difference is that this function returns Action object directly, not
+    dict {unit_id: action}.
+    """
+    results = run_action_on_units([unit], action, **params)
+    return results[unit.entity_id]
+
+
 def parse_charm_name(charm_url: str) -> str:
     """Parse charm name from full charm url.
 
diff --git a/juju_verify/verifiers/base.py b/juju_verify/verifiers/base.py
index 7057d88..504c15f 100644
--- a/juju_verify/verifiers/base.py
+++ b/juju_verify/verifiers/base.py
@@ -201,42 +201,12 @@ class BaseVerifier:
             err = VerificationError('Verification failed: {}'.format(exc))
             raise err from exc
 
-    def run_action_on_all(self, action: str,
-                          **params: str) -> Dict[str, Action]:
+    def run_action_on_all(self, action: str, **params: str) -> Dict[str, Action]:
         """Run juju action on all units in self.units.
 
         For more info, see docstring for 'run_action_on_units'.
         """
-        return self.run_action_on_units(self.unit_ids, action, **params)
-
-    def run_action_on_unit(self, unit: str, action: str,
-                           **params: str) -> Action:
-        """Run juju action on single unit.
-
-        For more info, see docstring for 'run_action_on_units'. The only
-        difference is that this function returns Action object directly, not
-        dict {unit_id: action}.
-        """
-        results = self.run_action_on_units([unit], action, **params)
-        return results[unit]
-
-    def run_action_on_units(self, units: List[str], action: str,
-                            **params: str) -> Dict[str, Action]:
-        """Run juju action on specified units.
-
-        Units are specified by string that must match Unit.entity_id in
-        self.units. All actions that are executed are also awaited, if any
-        of the actions fails, VerificationError is raised.
-
-        :param units: List of unit IDs on which to run action
-        :param action: Action to run on units
-        :param params: Additional parameters for the action
-        :return: Dict in format {unit_id: action} where unit_ids are strings
-                 provided in 'units' and actions are their matching,
-                 juju.Action objects that have been executed and awaited.
-        """
-        target_units = [self.unit_from_id(unit_id) for unit_id in units]
-        return run_action_on_units(target_units, action=action, **params)
+        return run_action_on_units(self.units, action, **params)
 
     def verify_shutdown(self) -> Result:
         """Child classes must override this method with custom implementation.
diff --git a/juju_verify/verifiers/nova_compute.py b/juju_verify/verifiers/nova_compute.py
index 1e08ba0..c04a971 100644
--- a/juju_verify/verifiers/nova_compute.py
+++ b/juju_verify/verifiers/nova_compute.py
@@ -19,6 +19,7 @@ import json
 import logging
 
 from juju_verify.utils.action import data_from_action
+from juju_verify.utils.unit import run_action_on_unit
 from juju_verify.verifiers.base import BaseVerifier
 from juju_verify.verifiers.result import aggregate_results, Result, Severity
 
@@ -55,8 +56,7 @@ class NovaCompute(BaseVerifier):
         target_nodes = [data_from_action(action, 'node-name')
                         for _, action in node_name_actions.items()]
 
-        nova_unit = self.units[0].entity_id
-        action = self.run_action_on_unit(nova_unit, 'list-compute-nodes')
+        action = run_action_on_unit(self.units[0], 'list-compute-nodes')
         compute_nodes = json.loads(data_from_action(action, 'compute-nodes'))
 
         affected_zones = {node['zone'] for node in compute_nodes
diff --git a/tests/unit/utils/test_unit.py b/tests/unit/utils/test_unit.py
index e519520..b92088b 100644
--- a/tests/unit/utils/test_unit.py
+++ b/tests/unit/utils/test_unit.py
@@ -17,7 +17,7 @@
 """Utils unit test suite."""
 import asyncio
 from typing import List
-from unittest.mock import MagicMock, call
+from unittest.mock import MagicMock, call, patch
 
 import pytest
 from juju.action import Action
@@ -30,7 +30,7 @@ from juju_verify.utils.unit import (
     verify_charm_unit,
     parse_charm_name,
     get_first_active_unit,
-    get_applications_names
+    get_applications_names, run_action_on_unit
 )
 
 
@@ -109,6 +109,14 @@ def test_run_action_on_units(mocker, model, all_units):
     assert str(exc.value) == expect_err
 
 
+@patch("juju_verify.utils.unit.run_action_on_units")
+def test_base_verifier_run_action_on_unit(mock_run_action_on_units, model):
+    """Test running action on single unit from the verifier."""
+    unit = model.units["nova-compute/0"]
+    run_action_on_unit(unit, "test")
+    mock_run_action_on_units.assert_called_with([unit], "test")
+
+
 @pytest.mark.parametrize("charm_url, exp_name", [
     ("cs:focal/nova-compute-141", "nova-compute"),
     ("cs:hacluster-74", "hacluster"),
diff --git a/tests/unit/verifiers/test_base.py b/tests/unit/verifiers/test_base.py
index d68a960..a0b3e03 100644
--- a/tests/unit/verifiers/test_base.py
+++ b/tests/unit/verifiers/test_base.py
@@ -17,7 +17,7 @@
 """Base class test suite."""
 import asyncio
 from unittest import mock
-from unittest.mock import MagicMock, PropertyMock, call
+from unittest.mock import MagicMock, PropertyMock
 
 import pytest
 from juju.model import Model
@@ -189,48 +189,13 @@ def test_base_verifier_unit_from_id():
 
 
 @mock.patch("juju_verify.verifiers.base.run_action_on_units")
-def test_base_verifier_run_action_on_unit(mock_run_action_on_units, mocker, model):
-    """Test running action on single unit from the verifier."""
-    # Put spy on unit_id resolution
-    id_to_unit = mocker.spy(BaseVerifier, 'unit_from_id')
-    units = list(model.units.values())
-
-    verifier = BaseVerifier(units)
-    verifier.run_action_on_unit(units[1].entity_id, "test")
-
-    id_to_unit.assert_has_calls([call(verifier, units[1].entity_id)])
-    mock_run_action_on_units.assert_called_with([units[1]], action="test")
-
-
[e-mail]("juju_verify.verifiers.base.run_action_on_units")
-def test_base_verifier_run_action_on_all_units(mock_run_action_on_units, mocker, model):
+def test_base_verifier_run_action_on_all_units(mock_run_action_on_units, model):
     """Test running action on all units in verifier."""
-    # Put spy on unit_id resolution
-    id_to_unit = mocker.spy(BaseVerifier, 'unit_from_id')
     units = list(model.units.values())
-
     verifier = BaseVerifier(units)
     verifier.run_action_on_all("test")
 
-    id_to_unit.assert_has_calls([call(verifier, unit.entity_id) for unit in units])
-    mock_run_action_on_units.assert_called_with(units, action="test")
-
-
[e-mail]("juju_verify.verifiers.base.run_action_on_units")
-def test_base_verifier_run_action_on_units(mock_run_action_on_units, mocker, model):
-    """Test running action on list of units and returning results."""
-    # Put spy on unit_id resolution
-    id_to_unit = mocker.spy(BaseVerifier, 'unit_from_id')
-    units = list(model.units.values())
-    run_on_units = [unit for id_, unit in model.units.items()
-                    if id_.startswith("nova-compute")]
-
-    verifier = BaseVerifier(units)
-    verifier.run_action_on_units([unit.entity_id for unit in run_on_units], "test")
-
-    id_to_unit.assert_has_calls(
-        [call(verifier, unit.entity_id) for unit in run_on_units])
-    mock_run_action_on_units.assert_called_with(run_on_units, action="test")
+    mock_run_action_on_units.assert_called_with(units, "test")
 
 
 def test_base_verifier_check_has_sub_machines(mocker):
diff --git a/tests/unit/verifiers/test_nova_compute.py b/tests/unit/verifiers/test_nova_compute.py
index 5d2bdff..58450af 100644
--- a/tests/unit/verifiers/test_nova_compute.py
+++ b/tests/unit/verifiers/test_nova_compute.py
@@ -23,7 +23,7 @@ from juju.model import Model
 from juju.unit import Unit
 from pytest import param
 
-from juju_verify.verifiers import NovaCompute
+from juju_verify.verifiers.nova_compute import NovaCompute
 from juju_verify.verifiers.result import Result, Severity, Partial
 
 
@@ -118,21 +118,18 @@ def test_nova_compute_empty_az(all_hosts, remove_hosts, host_state,
     mocker.patch.object(NovaCompute,
                         'run_action_on_all').return_value = action_results
 
-    # mock result 'list-compute-nodes' action. Number of nodes in zone
-    # is parametrized.
+    # mock result 'list-compute-nodes' action. Number of nodes in zone is parametrized.
     raw_compute_nodes = [{'host': host,
                           'zone': zone,
                           'state': host_state,
                           'status': host_status}
                          for host in host_pool[:all_hosts]]
 
-    compute_nodes_data = {'results': {
-        'compute-nodes': json.dumps(raw_compute_nodes)}}
+    compute_nodes_data = {'results': {'compute-nodes': json.dumps(raw_compute_nodes)}}
     mock_compute_node_result = MagicMock()
     mock_compute_node_result.data = compute_nodes_data
-    mocker.patch.object(NovaCompute,
-                        'run_action_on_unit'
-                        ).return_value = mock_compute_node_result
+    mocker.patch('juju_verify.verifiers.nova_compute.run_action_on_unit'
+                 ).return_value = mock_compute_node_result
 
     # run verifier
     verifier = NovaCompute(units)