From 99d50a2115fa26f1deda8b9ca0c4c45e0a995fe4 Mon Sep 17 00:00:00 2001
From: jneo8 <james.lin@canonical.com>
Date: Tue, 20 Feb 2024 11:37:41 +0800
Subject: [PATCH] refactor: Split IPMIStrategy to sensor, sel, and dcmi
 strategies

Fix #124
---
 src/hw_tools.py             | 30 ++++++++++++++++------
 tests/unit/test_hw_tools.py | 50 ++++++++++++++++++++++++++++++++++---
 2 files changed, 68 insertions(+), 12 deletions(-)

diff --git a/src/hw_tools.py b/src/hw_tools.py
index fa1e5171..7f0d1991 100644
--- a/src/hw_tools.py
+++ b/src/hw_tools.py
@@ -308,12 +308,6 @@ def check(self) -> bool:
 class IPMIStrategy(APTStrategyABC):
     """Strategy for installing ipmi."""
 
-    # Because IPMISTrategy now encompasses all of
-    # HWTool.IPMI_SENSOR, HWTool.IPMI_SEL and HWTool.IPMI_DCMI,
-    # we will need some refactoring here to avoid misleading log
-    # messages. The installation should be good since all of these
-    # tools require the same `freeipmi-tools` to be installed.
-    _name = HWTool.IPMI_SENSOR
     pkg = "freeipmi-tools"
 
     def install(self) -> None:
@@ -322,13 +316,31 @@ def install(self) -> None:
     def remove(self) -> None:
         # Skip removing because we afriad this cause dependency error
         # for other services on the same machine.
-        logger.info("IPMIStrategy skip removing %s", self.pkg)
+        logger.info("%s skip removing %s", self._name, self.pkg)
 
     def check(self) -> bool:
         """Check package status."""
         return check_deb_pkg_installed(self.pkg)
 
 
+class IPMISENSORStrategy(IPMIStrategy):
+    """Strategy for installing ipmi."""
+
+    _name = HWTool.IPMI_SENSOR
+
+
+class IPMISELStrategy(IPMIStrategy):
+    """Strategy for installing ipmi."""
+
+    _name = HWTool.IPMI_SEL
+
+
+class IPMIDCMIStrategy(IPMIStrategy):
+    """Strategy for installing ipmi."""
+
+    _name = HWTool.IPMI_DCMI
+
+
 class RedFishStrategy(StrategyABC):  # pylint: disable=R0903
     """Install strategy for redfish.
 
@@ -490,7 +502,9 @@ def strategies(self) -> List[StrategyABC]:
             SAS2IRCUStrategy(),
             SAS3IRCUStrategy(),
             SSACLIStrategy(),
-            IPMIStrategy(),
+            IPMISELStrategy(),
+            IPMIDCMIStrategy(),
+            IPMISENSORStrategy(),
             RedFishStrategy(),
         ]
 
diff --git a/tests/unit/test_hw_tools.py b/tests/unit/test_hw_tools.py
index 033e6156..ddf23bc2 100644
--- a/tests/unit/test_hw_tools.py
+++ b/tests/unit/test_hw_tools.py
@@ -22,7 +22,9 @@
     APTStrategyABC,
     HWToolHelper,
     InvalidCredentialsError,
-    IPMIStrategy,
+    IPMIDCMIStrategy,
+    IPMISELStrategy,
+    IPMISENSORStrategy,
     PercCLIStrategy,
     ResourceChecksumError,
     ResourceFileSizeZeroError,
@@ -673,11 +675,11 @@ def test_remove(self, mock_apt):
         mock_repos.disable.assert_not_called()
 
 
-class TestIPMIStrategy(unittest.TestCase):
+class TestIPMISENSORStrategy(unittest.TestCase):
     @mock.patch("apt_helpers.get_candidate_version")
     @mock.patch("apt_helpers.apt")
     def test_install(self, mock_apt, mock_candidate_version):
-        strategy = IPMIStrategy()
+        strategy = IPMISENSORStrategy()
         mock_candidate_version.return_value = "some-candidate-version"
         strategy.install()
 
@@ -687,7 +689,47 @@ def test_install(self, mock_apt, mock_candidate_version):
 
     @mock.patch("hw_tools.apt")
     def test_remove(self, mock_apt):
-        strategy = IPMIStrategy()
+        strategy = IPMISENSORStrategy()
+        strategy.remove()
+
+        mock_apt.remove_package.assert_not_called()
+
+
+class TestIPMISELStrategy(unittest.TestCase):
+    @mock.patch("apt_helpers.get_candidate_version")
+    @mock.patch("apt_helpers.apt")
+    def test_install(self, mock_apt, mock_candidate_version):
+        strategy = IPMISELStrategy()
+        mock_candidate_version.return_value = "some-candidate-version"
+        strategy.install()
+
+        mock_apt.add_package.assert_called_with(
+            "freeipmi-tools", version="some-candidate-version", update_cache=False
+        )
+
+    @mock.patch("hw_tools.apt")
+    def test_remove(self, mock_apt):
+        strategy = IPMISELStrategy()
+        strategy.remove()
+
+        mock_apt.remove_package.assert_not_called()
+
+
+class TestIPMIDCMIStrategy(unittest.TestCase):
+    @mock.patch("apt_helpers.get_candidate_version")
+    @mock.patch("apt_helpers.apt")
+    def test_install(self, mock_apt, mock_candidate_version):
+        strategy = IPMIDCMIStrategy()
+        mock_candidate_version.return_value = "some-candidate-version"
+        strategy.install()
+
+        mock_apt.add_package.assert_called_with(
+            "freeipmi-tools", version="some-candidate-version", update_cache=False
+        )
+
+    @mock.patch("hw_tools.apt")
+    def test_remove(self, mock_apt):
+        strategy = IPMIDCMIStrategy()
         strategy.remove()
 
         mock_apt.remove_package.assert_not_called()