From 01aa854cc24348458a6a186fc72bcdfb359883fd Mon Sep 17 00:00:00 2001
From: Robert Gildein <gildeinrobert@gmail.com>
Date: Thu, 29 Feb 2024 15:39:26 +0100
Subject: [PATCH] Skip to add an empty step (#260)

We do not need to have empty step in plan, so I added the if condition
to BaseStep.add step function to skip any empty step.

---------

Co-authored-by: Gabriel Cocenza <gabrielcocenza@gmail.com>
---
 cou/apps/base.py               | 18 ++++++++----------
 cou/steps/__init__.py          |  5 +++++
 tests/unit/steps/test_steps.py | 24 ++++++++++++++++++++----
 3 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/cou/apps/base.py b/cou/apps/base.py
index ade6a05b..06a883e6 100644
--- a/cou/apps/base.py
+++ b/cou/apps/base.py
@@ -499,18 +499,16 @@ def generate_upgrade_plan(self, target: OpenStackRelease) -> ApplicationUpgradeP
         :return: Full upgrade plan if the Application is able to generate it.
         :rtype: ApplicationUpgradePlan
         """
-        upgrade_steps = ApplicationUpgradePlan(
+        upgrade_plan = ApplicationUpgradePlan(
             description=f"Upgrade plan for '{self.name}' to {target}",
         )
-        all_steps = (
-            self.pre_upgrade_steps(target)
-            + self.upgrade_steps(target)
-            + self.post_upgrade_steps(target)
-        )
-        for step in all_steps:
-            if step:
-                upgrade_steps.add_step(step)
-        return upgrade_steps
+        upgrade_plan.sub_steps = [
+            *self.pre_upgrade_steps(target),
+            *self.upgrade_steps(target),
+            *self.post_upgrade_steps(target),
+        ]
+
+        return upgrade_plan
 
     def _get_upgrade_current_release_packages_step(self) -> PreUpgradeStep:
         """Get step for upgrading software packages to the latest of the current release.
diff --git a/cou/steps/__init__.py b/cou/steps/__init__.py
index 9d0f4a3c..fa2c7d73 100644
--- a/cou/steps/__init__.py
+++ b/cou/steps/__init__.py
@@ -168,6 +168,7 @@ def description(self, description: str) -> None:
         """
         if not description and self._coro:
             raise ValueError("Every coroutine should have a description")
+
         self._description = description
 
     @property
@@ -224,6 +225,10 @@ def add_step(self, step: BaseStep) -> None:
         if not isinstance(step, BaseStep):
             raise TypeError("Cannot add an upgrade step that is not derived from BaseStep")
 
+        if not step:
+            logger.debug("skipping adding empty step")
+            return
+
         self._sub_steps.append(step)
 
     def cancel(self, safe: bool = True) -> None:
diff --git a/tests/unit/steps/test_steps.py b/tests/unit/steps/test_steps.py
index 02cc36d6..93b7b3ea 100644
--- a/tests/unit/steps/test_steps.py
+++ b/tests/unit/steps/test_steps.py
@@ -109,12 +109,14 @@ def test_step_bool():
     sub_step = BaseStep(description="a.a")
     assert bool(sub_step) is False
 
-    plan.sub_steps = [sub_step]
+    plan.add_step(sub_step)
     assert bool(plan) is False
 
     # coroutine in the plan sub_steps tree
     sub_sub_step = BaseStep(description="a.a.a", coro=mock_coro("a.a.a"))
-    sub_step.sub_steps = [sub_sub_step]
+    sub_step.add_step(sub_sub_step)
+    plan.add_step(sub_step)
+
     assert bool(sub_sub_step) is True
     assert bool(sub_step) is True
     assert bool(plan) is True
@@ -202,6 +204,16 @@ def test_step_add_step():
     exp_sub_steps = 3
     plan = BaseStep(description="plan")
     for i in range(exp_sub_steps):
+        plan.add_step(BaseStep(description=f"sub-step-{i}", coro=mock_coro()))
+
+    assert len(plan.sub_steps) == exp_sub_steps
+
+
+def test_step_add_step_skipping_empty():
+    """Test BaseStep skipping to add empty sub steps."""
+    exp_sub_steps = 0
+    plan = BaseStep(description="plan")
+    for i in range(3):
         plan.add_step(BaseStep(description=f"sub-step-{i}"))
 
     assert len(plan.sub_steps) == exp_sub_steps
@@ -219,9 +231,13 @@ def test_step_add_step_failed():
 def test_step_cancel_safe():
     """Test step safe cancel."""
     plan = BaseStep(description="plan")
-    plan.sub_steps = sub_steps = [BaseStep(description=f"sub-{i}") for i in range(10)]
+    plan.sub_steps = sub_steps = [
+        BaseStep(description=f"sub-{i}", coro=mock_coro()) for i in range(10)
+    ]
     # add sub-sub-steps to one sub-step
-    sub_steps[0].sub_steps = [BaseStep(description=f"sub-0.{i}") for i in range(3)]
+    sub_steps[0].sub_steps = [
+        BaseStep(description=f"sub-0.{i}", coro=mock_coro()) for i in range(3)
+    ]
 
     plan.cancel()