From e5c51d924fda8b9f00b714fdda2a2c03daff0a1e Mon Sep 17 00:00:00 2001
From: "Owen W. Taylor" <otaylor@fishsoup.net>
Date: Mon, 9 Nov 2020 10:53:15 -0500
Subject: [PATCH] Don't traceback on failed builds

Just failing a build isn't a reason to die with a traceback - exit with
a status instead.

Fixes: #1364
---
 module_build_service/manage.py          | 15 ++++++---------
 module_build_service/scheduler/local.py |  6 +++++-
 tests/test_manage.py                    | 16 ++++++++++------
 3 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/module_build_service/manage.py b/module_build_service/manage.py
index 925b5031..afe458a4 100755
--- a/module_build_service/manage.py
+++ b/module_build_service/manage.py
@@ -5,6 +5,7 @@ from functools import wraps
 import getpass
 import logging
 import os
+import sys
 import textwrap
 
 import flask_migrate
@@ -194,15 +195,11 @@ def build_module_locally(
 
         module_build_ids = [build.id for build in module_builds]
 
-    module_build_service.scheduler.local.main(module_build_ids)
-
-    has_failed_module = db_session.query(models.ModuleBuild).filter(
-        models.ModuleBuild.id.in_(module_build_ids),
-        models.ModuleBuild.state == models.BUILD_STATES["failed"],
-    ).count() > 0
-
-    if has_failed_module:
-        raise RuntimeError("Module build failed")
+    try:
+        module_build_service.scheduler.local.main(module_build_ids)
+    except module_build_service.scheduler.local.BuildFailed as e:
+        logging.error("%s", e)
+        sys.exit(1)
 
 
 @manager.option(
diff --git a/module_build_service/scheduler/local.py b/module_build_service/scheduler/local.py
index 569d6bd1..79a91ae6 100644
--- a/module_build_service/scheduler/local.py
+++ b/module_build_service/scheduler/local.py
@@ -21,6 +21,10 @@ module build and running tests within test_build.py particularly.
 __all__ = ["main"]
 
 
+class BuildFailed(Exception):
+    pass
+
+
 def raise_for_failed_build(module_build_ids):
     """
     Raises an exception if any module build from `module_build_ids` list is in failed state.
@@ -36,7 +40,7 @@ def raise_for_failed_build(module_build_ids):
             modules_failed_handler("fake_msg_id", build.id, "failed")
             has_failed_build = True
     if has_failed_build:
-        raise ValueError("Local module build failed.")
+        raise BuildFailed("Local module build failed.")
 
 
 def main(module_build_ids):
diff --git a/tests/test_manage.py b/tests/test_manage.py
index 6d3026c8..882549e9 100644
--- a/tests/test_manage.py
+++ b/tests/test_manage.py
@@ -2,6 +2,7 @@
 # SPDX-License-Identifier: MIT
 from __future__ import absolute_import
 
+import logging
 from mock import patch
 import pytest
 
@@ -205,14 +206,14 @@ class TestCommandBuildModuleLocally:
         args, _ = logging.error.call_args_list[1]
         assert "Use '-s module_name:module_stream' to choose the stream" == args[0]
 
-    def test_module_build_failed(self):
+    def test_module_build_failed(self, caplog):
         cli_cmd = [
             "mbs-manager", "build_module_locally",
             "--set-stream", "platform:f28",
             "--file", staged_data_filename("testmodule-local-build.yaml")
         ]
 
-        def main_side_effect(module_build_ids):
+        def init_side_effect(*args):
             build = db_session.query(models.ModuleBuild).filter(
                 models.ModuleBuild.name == "testmodule-local-build"
             ).first()
@@ -222,7 +223,10 @@ class TestCommandBuildModuleLocally:
         # We don't run consumer actually, but it could be patched to mark some
         # module build failed for test purpose.
 
-        with patch("module_build_service.scheduler.local.main",
-                   side_effect=main_side_effect):
-            with pytest.raises(RuntimeError, match="Module build failed"):
-                self._run_manager_wrapper(cli_cmd)
+        with patch("module_build_service.scheduler.local.modules_init_handler",
+                   side_effect=init_side_effect):
+            self._run_manager_wrapper(cli_cmd)
+
+        r = [r for r in caplog.records if r.levelno == logging.ERROR
+             and "Local module build failed" in r.getMessage()]
+        assert len(r) > 0