add support for buffering log comments before MR hook initialization,…

test/common/logger_test.py

@@ -10,6 +10,7 @@ def tearDown(self):
         from valhalla.common import logger
         logger.MR_HOOK = None
         logger.MR_HOOK_COMMENTS_COUNT = 0
+        logger.PENDING_MR_COMMENTS = []
 
     @patch('builtins.print')
     def test_info(self, mock_print):
@@ -153,6 +154,48 @@ def test_exit_after_50_comments(self, mock_exit, mock_print):
         # cleanup
         logger.MR_HOOK_COMMENTS_COUNT = 0
 
+    @patch('builtins.print')
+    def test_pending_warn_and_error_flushed_when_hook_set_later(self, mock_print):
+        # given: errors logged before MR hook is initialized
+        warn("early warn")
+        error("early error")
+
+        # when: MR hook is initialized later
+        mr_hook = MagicMock()
+        init_logger_mr_hook(mr_hook)
+
+        # then: pending comments are flushed in order
+        mr_hook.add_comment.assert_any_call("[WARN] early warn")
+        mr_hook.add_comment.assert_any_call("[ERROR] early error")
+        self.assertEqual(mr_hook.add_comment.call_count, 2)
+
+    @patch('builtins.print')
+    def test_pending_buffer_cleared_after_flush(self, mock_print):
+        # given: an early error and a hook set up
+        error("early error")
+        mr_hook = MagicMock()
+        init_logger_mr_hook(mr_hook)
+
+        # when: a new hook is set up afterwards
+        mr_hook.reset_mock()
+        another_hook = MagicMock()
+        init_logger_mr_hook(another_hook)
+
+        # then: previously flushed comments are not re-flushed
+        another_hook.add_comment.assert_not_called()
+
+    @patch('builtins.print')
+    def test_info_not_buffered_before_hook(self, mock_print):
+        # given: an info log before MR hook is initialized
+        info("informational")
+
+        # when: MR hook is initialized later
+        mr_hook = MagicMock()
+        init_logger_mr_hook(mr_hook)
+
+        # then: info messages are never sent to MR
+        mr_hook.add_comment.assert_not_called()
+
     @patch('builtins.print')
     def test_resolve_author_in_log(self, mock_print):
         # given:

test/main_start_test.py

@@ -53,3 +53,41 @@ def __init__(self, text):
             self.assertEqual(args[2], "1.2.3")
             # merge request config is default and disabled in our test YAML
             self.assertEqual(mock_create_mr.call_count, 1)
+
+    def test_start_creates_mr_and_posts_error_when_version_empty(self):
+        # given: version is empty and from_config does not resolve it (e.g. command failed)
+        mock_vtr = MagicMock()
+        mock_vtr.is_version_empty.return_value = True
+        mock_vtr.get_config_file_path.return_value = "test/resources/valhalla.yml"
+
+        provider_hook = MagicMock()
+
+        def _mock_requests_get(url):
+            class R:
+                status_code = 200
+
+                def __init__(self, text):
+                    self.text = text
+
+            with open("test/resources/valhalla-extended.yml", "r") as f:
+                return R(f.read())
+
+        with patch('valhalla.main.__version_to_release', return_value=mock_vtr), \
+                patch('valhalla.extends.valhalla_extends.requests.get', side_effect=_mock_requests_get), \
+                patch('valhalla.main.get_valhalla_token', return_value='TOKEN'), \
+                patch('os.environ.get', side_effect=fake_environ_get), \
+                patch('valhalla.ci_provider.git_host.GitHost.get_author', return_value='AUTHOR'), \
+                patch('valhalla.main.create_merge_request', return_value=provider_hook) as mock_create_mr:
+            from valhalla.main import start
+
+            # when: running start should exit with error
+            with self.assertRaises(SystemExit):
+                start()
+
+            # then: MR is created so the error is posted as a comment
+            self.assertEqual(mock_create_mr.call_count, 1)
+            # The "Version to release is empty" error is posted to the MR
+            posted = "\n".join(
+                call_args[0][0] for call_args in provider_hook.add_comment.call_args_list
+            )
+            self.assertIn("Version to release is empty", posted)

valhalla/common/logger.py

@@ -3,6 +3,7 @@
 TOKEN: str = "not_set"
 MR_HOOK: MergeRequestHook | None = None
 MR_HOOK_COMMENTS_COUNT: int = 0
+PENDING_MR_COMMENTS: list = []
 
 
 # Allows to hide sensitive data
@@ -12,14 +13,26 @@ def init_logger(token: str):
 
 
 def init_logger_mr_hook(mr_hook: MergeRequestHook):
-    global MR_HOOK
+    global MR_HOOK, MR_HOOK_COMMENTS_COUNT, PENDING_MR_COMMENTS
     MR_HOOK = mr_hook
 
+    if MR_HOOK is None or not PENDING_MR_COMMENTS:
+        PENDING_MR_COMMENTS = []
+        return
+
+    pending = PENDING_MR_COMMENTS
+    PENDING_MR_COMMENTS = []
+    for comment in pending:
+        if MR_HOOK_COMMENTS_COUNT >= 50:
+            break
+        MR_HOOK.add_comment(comment)
+        MR_HOOK_COMMENTS_COUNT += 1
+
 
 def log_message(level, msg):
     from valhalla.common import resolver
     import sys
-    global TOKEN, MR_HOOK, MR_HOOK_COMMENTS_COUNT
+    global TOKEN, MR_HOOK, MR_HOOK_COMMENTS_COUNT, PENDING_MR_COMMENTS
     msg = str(msg)
     msg = resolver.resolve(msg, suppress_log=True)
     msg = msg.replace(TOKEN, "*" * len(TOKEN))
@@ -34,14 +47,20 @@ def log_message(level, msg):
         else:
             formatted_msg += "  \n" + line_to_print
 
-    if MR_HOOK is not None and (level == "WARN" or level == "ERROR"):
-        if MR_HOOK_COMMENTS_COUNT >= 50:
-            error_msg = f"[ERROR] Too many comments added to Merge Request (limit: 50). Please fix previous warnings."
-            MR_HOOK.add_comment(error_msg)
-            print(error_msg)
-            sys.exit(1)
-        MR_HOOK.add_comment(formatted_msg)
-        MR_HOOK_COMMENTS_COUNT += 1
+    if level != "WARN" and level != "ERROR":
+        return
+
+    if MR_HOOK is None:
+        PENDING_MR_COMMENTS.append(formatted_msg)
+        return
+
+    if MR_HOOK_COMMENTS_COUNT >= 50:
+        error_msg = f"[ERROR] Too many comments added to Merge Request (limit: 50). Please fix previous warnings."
+        MR_HOOK.add_comment(error_msg)
+        print(error_msg)
+        sys.exit(1)
+    MR_HOOK.add_comment(formatted_msg)
+    MR_HOOK_COMMENTS_COUNT += 1
 
 
 def info(msg):

valhalla/main.py

@@ -78,6 +78,12 @@ def start():
         version_to_release.from_config(config)
 
     if version_to_release.is_version_empty():
+        try:
+            mr_hook = create_merge_request(git_host, config.merge_request)
+        except Exception as e:
+            info(f"Could not create merge request for error reporting: {e}")
+            mr_hook = MergeRequestHook.Skip()
+        init_logger_mr_hook(mr_hook)
         error(
             f"Version to release is empty, exiting! Create branch with name {BASE_PREFIX}X.X.X or just {BASE_PREFIX}\n"
             f"and define in valhalla.yml version to release. You can also you VALHALLA_RELEASE_CMD to define it.\n"