MAINT: Raising an error on actions rather than returning none.

To help users understand if the action has failed. Additional code refactoring.

lib/openstack_api/openstack_service.py

@@ -17,6 +17,7 @@ def disable_service(
     :param hypervisor_name: The name or ID of the hypervisor.
     :param service_binary: The name of the service.
     :param disabled_reason: The reason for disabling the service.
+    :return: Returns the Service object.
     """
 
     if service.status == "enabled":
@@ -26,7 +27,9 @@ def disable_service(
             binary=service_binary,
             disabled_reason=disabled_reason,
         )
-    return None
+    raise RuntimeError(
+        f"Failed to disable {service_binary} on {hypervisor_name}. Already disabled."
+    )
 
 
 def enable_service(
@@ -41,6 +44,7 @@ def enable_service(
     :param service: The instance of the service class.
     :param hypervisor_name: The name or ID of the hypervisor.
     :param service_binary: The name of the service.
+    :return: Returns the Service object.
     """
 
     if service.status == "disabled":

tests/lib/openstack_api/test_openstack_service.py

@@ -1,11 +1,11 @@
 from unittest.mock import MagicMock, NonCallableMagicMock
+import pytest
 from openstack_api.openstack_service import disable_service, enable_service
 
 
 def test_disable_service_function_disables_service_when_enabled():
     """tests that openstack disables the service when it is currently enabled"""
 
-    # mock params
     mock_conn = MagicMock()
     mock_service = MagicMock()
     mock_hypervisor_name = NonCallableMagicMock()
@@ -14,7 +14,6 @@ def test_disable_service_function_disables_service_when_enabled():
 
     mock_service.status = "enabled"
 
-    # run the function
     res = disable_service(
         mock_conn,
         mock_service,
@@ -23,22 +22,19 @@ def test_disable_service_function_disables_service_when_enabled():
         mock_disable_reason,
     )
 
-    # assert expectations
-    # assert conn.compute.disable_service was called appropriately
     mock_conn.compute.disable_service.assert_called_once_with(
         mock_service,
         host=mock_hypervisor_name,
         binary=mock_service_binary,
         disabled_reason=mock_disable_reason,
     )
-    # test that function returns == conn.compute.disable_service.return_value
+
     assert res == mock_conn.compute.disable_service.return_value
 
 
 def test_disable_service_function_returns_none_when_disabled():
     """tests that openstack returns None when the service is currently disabled"""
 
-    # mock params
     mock_conn = MagicMock()
     mock_service = MagicMock()
     mock_hypervisor_name = NonCallableMagicMock()
@@ -47,36 +43,30 @@ def test_disable_service_function_returns_none_when_disabled():
 
     mock_service.status = "disabled"
 
-    # run the function
-    res = disable_service(
-        mock_conn,
-        mock_service,
-        mock_hypervisor_name,
-        mock_service_binary,
-        mock_disable_reason,
-    )
-
-    # assert expectations
-    assert res is None
+    with pytest.raises(RuntimeError):
+        disable_service(
+            mock_conn,
+            mock_service,
+            mock_hypervisor_name,
+            mock_service_binary,
+            mock_disable_reason,
+        )
 
 
 def test_enable_service_function_enables_service_when_disabled():
     """tests that openstack enables the service when it is currently disabled"""
 
-    # mock params
     mock_conn = MagicMock()
     mock_service = MagicMock()
     mock_hypervisor_name = NonCallableMagicMock()
     mock_service_binary = NonCallableMagicMock()
 
     mock_service.status = "disabled"
 
-    # run the function
     res = enable_service(
         mock_conn, mock_service, mock_hypervisor_name, mock_service_binary
     )
 
-    # assert expectations
     mock_conn.compute.enable_service.assert_called_once_with(
         mock_service,
         host=mock_hypervisor_name,
@@ -89,18 +79,14 @@ def test_enable_service_function_enables_service_when_disabled():
 def test_enable_service_function_returns_none_when_enabled():
     """tests that openstack returns none when the service is currently enabled"""
 
-    # mock params
     mock_conn = MagicMock()
     mock_service = MagicMock()
     mock_hypervisor_name = NonCallableMagicMock()
     mock_service_binary = NonCallableMagicMock()
 
     mock_service.status = "enabled"
 
-    # run the function
-    res = enable_service(
-        mock_conn, mock_service, mock_hypervisor_name, mock_service_binary
-    )
-
-    # assert expectations
-    assert res is None
+    with pytest.raises(RuntimeError):
+        enable_service(
+            mock_conn, mock_service, mock_hypervisor_name, mock_service_binary
+        )

tests/lib/workflows/test_hv_service_actions.py

@@ -4,21 +4,17 @@
 
 @patch("workflows.hv_service_actions.disable_service")
 def test_service_disabled_successfully(mock_hv_service_disable):
+    """test that hv_service_disable disables an openstack service"""
 
-    # test that hv_service_disable disables an openstack service
-
-    # mock params
     mock_conn = MagicMock()
     mock_hypervisor_name = NonCallableMagicMock()
     mock_service_binary = NonCallableMagicMock()
     mock_disabled_reason = NonCallableMagicMock()
 
-    # run the function
     hv_service_disable(
         mock_conn, mock_hypervisor_name, mock_service_binary, mock_disabled_reason
     )
 
-    # assert your expectations
     mock_conn.compute.find_service.assert_called_once_with(
         mock_service_binary, ignore_missing=False, host=mock_hypervisor_name
     )
@@ -34,18 +30,14 @@ def test_service_disabled_successfully(mock_hv_service_disable):
 
 @patch("workflows.hv_service_actions.enable_service")
 def test_hv_service_enabled_successfully(mock_hv_service_enable):
+    """test that hv_service_enable enables an openstack service"""
 
-    # test that hv_service_enable enables an openstack service
-
-    # mock params
     mock_conn = MagicMock()
     mock_hypervisor_name = NonCallableMagicMock()
     mock_service_binary = NonCallableMagicMock()
 
-    # Run function
     hv_service_enable(mock_conn, mock_hypervisor_name, mock_service_binary)
 
-    # assert expectations
     mock_conn.compute.find_service.assert_called_once_with(
         mock_service_binary, ignore_missing=False, host=mock_hypervisor_name
     )