Repository: osbuild/osbuild
Pull request #535 (2020-10-27 09:21:56)

osbuild: always return exit code
osbuild_cli() sometimes returned an exit code, but at the end called
sys.exit() directly. The idea was probably to always return the code
with which the executable should exit.

Make this consistent and call sys.exit() in __main__.py, with the value
returned by osbuild_cli().

gicmo: :heart: very cool! I wonder if there is a good way to test this, so that we catch this the next time we restructure the code ;)
gicmo: > heart very cool! I wonder if there is a good way to test this, so that we catch this the next time we restructure the code ;)

I guess a invalid manifest and an invalid checksum would at least catch some of the cases.

Patch:
diff --git a/osbuild/__main__.py b/osbuild/__main__.py
index 7d0c70f4..f5275b01 100755
--- a/osbuild/__main__.py
+++ b/osbuild/__main__.py
@@ -4,9 +4,11 @@ This specifies the entrypoint of the osbuild module when run as executable. For
 compatibility we will continue to run the CLI.
 """
 
+import sys
 
 from osbuild.main_cli import osbuild_cli as main
 
 
 if __name__ == "__main__":
-    main()
+    r = main()
+    sys.exit(r)
diff --git a/osbuild/main_cli.py b/osbuild/main_cli.py
index dba0d45b..acd19d72 100644
--- a/osbuild/main_cli.py
+++ b/osbuild/main_cli.py
@@ -172,4 +172,4 @@ def osbuild_cli():
             print()
             print(f"{RESET}{BOLD}{RED}Failed{RESET}")
 
-    sys.exit(0 if r["success"] else 1)
+    return 0 if r["success"] else 1
diff --git a/schutzbot/Jenkinsfile b/schutzbot/Jenkinsfile
index d8e2ab94..ec00d015 100644
--- a/schutzbot/Jenkinsfile
+++ b/schutzbot/Jenkinsfile
@@ -227,9 +227,6 @@ void preserve_logs(test_slug) {
     // Save the systemd journal.
     sh "sudo journalctl --boot > systemd-journald.log"
 
-    // Find any AVCs in the audit log and save those.
-    sh "sudo grep AVC /var/log/audit/audit.log > selinux-avc.log"
-
     // Make a directory for the log files and move the logs there.
     sh "mkdir ${test_slug} && mv *.log *.jpg ${test_slug}/ || true"
 


Test patch:
diff --git a/test/run/test_executable.py b/test/run/test_executable.py
new file mode 100644
index 00000000..72a3c1a7
--- /dev/null
+++ b/test/run/test_executable.py
@@ -0,0 +1,30 @@
+#
+# Runtime Tests for the `osbuild` executable
+#
+
+import json
+import subprocess
+import unittest
+
+from .. import test
+
+
+class TestExecutable(unittest.TestCase):
+    def setUp(self):
+        self.osbuild = test.OSBuild(self)
+
+    def test_invalid_manifest(self):
+        invalid = json.dumps({"foo": 42})
+
+        with self.osbuild as osb, self.assertRaises(subprocess.CalledProcessError) as e:
+            osb.compile(invalid, check=True)
+
+        self.assertEqual(e.exception.returncode, 2)
+
+    def test_invalid_checkpoint(self):
+        manifest = json.dumps({})
+
+        with self.osbuild as osb, self.assertRaises(subprocess.CalledProcessError) as e:
+            osb.compile(manifest, checkpoints=["f44f76973fb92446a2a33bfdb401361a47f70497"], check=True)
+
+        self.assertEqual(e.exception.returncode, 1)
diff --git a/test/test.py b/test/test.py
index d5173143..eacd5f51 100644
--- a/test/test.py
+++ b/test/test.py
@@ -269,7 +269,7 @@ class OSBuild(contextlib.AbstractContextManager):
         print(data_stderr)
         print("-- END ---------------------------------")
 
-    def compile(self, data_stdin, output_dir=None, checkpoints=None):
+    def compile(self, data_stdin, output_dir=None, checkpoints=None, check=False):
         """Compile an Artifact
 
         This takes a manifest as `data_stdin`, executes the pipeline, and
@@ -280,6 +280,10 @@ class OSBuild(contextlib.AbstractContextManager):
         the output_dir parameter. If it's set to None, a temporary directory
         is used and thus the caller cannot access the built artifact.
 
+        `check` determines what happens when running osbuild fails. If it is
+        true, subprocess.CalledProcessError is raised. Otherwise, osbuild's
+        output is printed to stdout and a test assertion is raised.
+
         Returns the build result as dictionary.
         """
 
@@ -289,7 +293,7 @@ class OSBuild(contextlib.AbstractContextManager):
             output_dir_context = contextlib.nullcontext(output_dir)
 
         with output_dir_context as osbuild_output_dir:
-            cmd_args = []
+            cmd_args = ["python3", "-m", "osbuild"]
 
             cmd_args += ["--json"]
             cmd_args += ["--libdir", "."]
@@ -299,13 +303,15 @@ class OSBuild(contextlib.AbstractContextManager):
             for c in (checkpoints or []):
                 cmd_args += ["--checkpoint", c]
 
+            cmd_args += ["-"]
+
             # Spawn the `osbuild` executable, feed it the specified data on
             # `STDIN` and wait for completion. If we are interrupted, we always
             # wait for `osbuild` to shut down, so we can clean up its file-system
             # trees (they would trigger `EBUSY` if we didn't wait).
             try:
                 p = subprocess.Popen(
-                    ["python3", "-m", "osbuild"] + cmd_args + ["-"],
+                    cmd_args,
                     encoding="utf-8",
                     stdin=subprocess.PIPE,
                     stdout=subprocess.PIPE,
@@ -316,8 +322,10 @@ class OSBuild(contextlib.AbstractContextManager):
                 p.wait()
                 raise
 
-        # If execution failed, print results to `STDOUT`.
+        # If execution failed, raise exception or print results to `STDOUT`.
         if p.returncode != 0:
+            if check:
+                raise subprocess.CalledProcessError(p.returncode, cmd_args, data_stdout, data_stderr)
             self._print_result(p.returncode, data_stdout, data_stderr)
             self._unittest.assertEqual(p.returncode, 0)