From 3fbd0b2a7307089bd36a7150f90eb7026cbe198f Mon Sep 17 00:00:00 2001
From: Michael Vogt <michael.vogt@gmail.com>
Date: Wed, 22 Nov 2023 11:08:47 +0100
Subject: [PATCH] monitor: tweak/simplify Progress

Tweak the Progress class to be simpler. Given that progress does
not need to support arbitrary depth but only has a single level
the class now just exposes "sub_progress" to the caller.

When the main progress is advanced the sub_progress is now fully
deleted instead of just reset. The rational is that when the main
progress is done and advances a step it is very likely that a
new sub_progress is required and it's most likely an error if
the same sub_progress will get re-used.

This means that `reset()` can be removed as it's not used anymore
(and YAGNI). We can add it back when we have a use-case.

It also change the code so that "total" starts with 0 instead
of `None` (principle of least surprise). This means that now
`progress.incr()` is called in the JSONSeqMonitor() for
`finish()` and `result()` to indicate that the pipeline/stage
is finished.
---
 osbuild/monitor.py       | 60 +++++++++++++++++++++-------------------
 test/mod/test_monitor.py | 18 ++++++------
 2 files changed, 42 insertions(+), 36 deletions(-)

diff --git a/osbuild/monitor.py b/osbuild/monitor.py
index b1fec5d1..05d16efa 100644
--- a/osbuild/monitor.py
+++ b/osbuild/monitor.py
@@ -113,30 +113,29 @@ class Context:
 
 
 class Progress:
+    """Progress represents generic progress information.
+
+    A progress can contain a sub_progress to track more
+    nested progresses. Any increment of a parent progress
+    will the reset the sub_progress to None and a new
+    sub_progress needs to be provided.
+
+    Keyword arguments:
+    name  -- user visible name for the progress
+    total -- total steps required to finish the progress
+    """
+
     def __init__(self, name: str, total: int):
         self.name = name
         self.total = total
-        self.done = None
-        self._sub_progress: Optional[Progress] = None
+        self.done = 0
+        self.sub_progress: Optional[Progress] = None
 
-    def incr(self, depth=0):
-        if depth > 0:
-            self._sub_progress.incr(depth - 1)
-        else:
-            if self.done is None:
-                self.done = 0
-            else:
-                self.done += 1
-            if self._sub_progress:
-                self._sub_progress.reset()
-
-    def reset(self):
-        self.done = None
-        if self._sub_progress:
-            self._sub_progress.reset()
-
-    def sub_progress(self, prog: "Progress"):
-        self._sub_progress = prog
+    def incr(self):
+        """Increment the "done" count"""
+        self.done += 1
+        if self.sub_progress:
+            self.sub_progress = None
 
     def as_dict(self):
         d = {
@@ -144,8 +143,8 @@ class Progress:
             "total": self.total,
             "done": self.done,
         }
-        if self._sub_progress:
-            d["progress"] = self._sub_progress.as_dict()
+        if self.sub_progress:
+            d["progress"] = self.sub_progress.as_dict()
         return d
 
 
@@ -299,23 +298,28 @@ class JSONSeqMonitor(BaseMonitor):
         self._progress = Progress("pipelines", len(manifest.pipelines))
         self._context = Context(origin="org.osbuild")
 
-    def result(self, result):
-        pass
+    # result is for modules
+    def result(self, result: osbuild.pipeline.BuildResult):
+        # TODO: check pipeline id?
+        if self._progress.sub_progress:
+            self._progress.sub_progress.incr()
 
     def begin(self, pipeline: osbuild.Pipeline):
         self._context.set_pipeline(pipeline)
-        self._progress.sub_progress(Progress("stages", len(pipeline.stages)))
+        self._progress.sub_progress = Progress("stages", len(pipeline.stages))
+
+    # finish is for pipelines
+    def finish(self, result: dict):
         self._progress.incr()
 
     def stage(self, stage: osbuild.Stage):
         self._module(stage)
 
-    def assembler(self, assembler):
+    def assembler(self, assembler: osbuild.Stage):
         self._module(assembler)
 
-    def _module(self, module):
+    def _module(self, module: osbuild.Stage):
         self._context.set_stage(module)
-        self._progress.incr(depth=1)
 
     def log(self, message, origin: Optional[str] = None):
         oo = self._context.origin
diff --git a/test/mod/test_monitor.py b/test/mod/test_monitor.py
index 2e17101f..b7c4507e 100644
--- a/test/mod/test_monitor.py
+++ b/test/mod/test_monitor.py
@@ -160,25 +160,27 @@ def test_context():
 
 def test_progress():
     prog = Progress("test", total=12)
-    subprog = Progress("test-sub1", total=3)
-    prog.sub_progress(subprog)
-    assert prog.done is None  # starts with None until the first incr()
-    prog.incr()
-
-    prog.incr(depth=1)
+    prog.sub_progress = Progress("test-sub1", total=3)
+    # we start empty
     progdict = prog.as_dict()
     assert progdict["done"] == 0
     assert progdict["progress"]["done"] == 0
 
-    prog.incr(depth=1)
+    # incr a sub_progress only affect sub_progress
+    prog.sub_progress.incr()
     progdict = prog.as_dict()
     assert progdict["done"] == 0
     assert progdict["progress"]["done"] == 1
 
+    prog.sub_progress.incr()
+    progdict = prog.as_dict()
+    assert progdict["done"] == 0
+    assert progdict["progress"]["done"] == 2
+
     prog.incr()
     progdict = prog.as_dict()
     assert progdict["done"] == 1
-    assert progdict["progress"]["done"] is None, "sub-progress did not reset"
+    assert progdict.get("progress") == None, "sub-progress did not reset"
 
 
 def test_json_progress_monitor():