[live-images] Code cleanup

This patch removes some duplicated variables that get passed on to the
build thread. It also moves creation of the command for generating image
manifest closer to where it is used. Finally it adds tests for the
thread.

Signed-off-by: Lubomír Sedlář <lsedlar@redhat.com>

pungi/phases/live_images.py

@@ -82,7 +82,6 @@ class LiveImagesPhase(PhaseBase):
 
     def run(self):
         symlink_isos_to = self.compose.conf.get("symlink_isos_to", None)
-        iso = IsoWrapper()
         commands = []
 
         for variant in self.compose.variants.values():
@@ -100,15 +99,12 @@ class LiveImagesPhase(PhaseBase):
                 cmd = {
                     "name": None,
                     "version": None,
-                    "arch": arch,
-                    "variant": variant,
                     "iso_path": None,
                     "wrapped_rpms_path": iso_dir,
                     "build_arch": arch,
                     "ks_file": ks_file,
                     "specfile": None,
                     "scratch": False,
-                    "cmd": [],
                     "label": "",  # currently not used
                 }
                 cmd["repos"] = [translate_path(
@@ -148,17 +144,7 @@ class LiveImagesPhase(PhaseBase):
                     self.compose.log_warning("Skipping creating live image, it already exists: %s" % iso_path)
                     continue
                 cmd["iso_path"] = iso_path
-                iso_name = os.path.basename(iso_path)
 
-                # Additional commands
-
-                chdir_cmd = "cd %s" % pipes.quote(iso_dir)
-                cmd["cmd"].append(chdir_cmd)
-
-                # create iso manifest
-                cmd["cmd"].append(iso.get_manifest_cmd(iso_name))
-
-                cmd["cmd"] = " && ".join(cmd["cmd"])
                 commands.append((cmd, variant, arch))
 
         for (cmd, variant, arch) in commands:
@@ -185,7 +171,7 @@ class CreateLiveImageThread(WorkerThread):
     def process(self, item, num):
         compose, cmd, variant, arch = item
         try:
-            self.worker(compose, cmd, num)
+            self.worker(compose, cmd, variant, arch, num)
         except:
             if not compose.can_fail(variant, arch, 'live'):
                 raise
@@ -194,10 +180,10 @@ class CreateLiveImageThread(WorkerThread):
                        % (variant.uid, arch))
                 self.pool.log_info(msg)
 
-    def worker(self, compose, cmd, num):
+    def worker(self, compose, cmd, variant, arch, num):
-        log_file = compose.paths.log.log_file(cmd["arch"], "createiso-%s" % os.path.basename(cmd["iso_path"]))
+        log_file = compose.paths.log.log_file(arch, "createiso-%s" % os.path.basename(cmd["iso_path"]))
 
-        msg = "Creating ISO (arch: %s, variant: %s): %s" % (cmd["arch"], cmd["variant"], os.path.basename(cmd["iso_path"]))
+        msg = "Creating ISO (arch: %s, variant: %s): %s" % (arch, variant, os.path.basename(cmd["iso_path"]))
         self.pool.log_info("[BEGIN] %s" % msg)
 
         koji_wrapper = KojiWrapper(compose.conf["koji_profile"])
@@ -232,11 +218,19 @@ class CreateLiveImageThread(WorkerThread):
             for rpm_path in rpm_paths:
                 shutil.copy2(rpm_path, cmd["wrapped_rpms_path"])
 
-        # write manifest
+        self._write_manifest(cmd['iso_path'])
-        run(cmd["cmd"])
 
         self.pool.log_info("[DONE ] %s" % msg)
 
+    def _write_manifest(self, iso_path):
+        """Generate manifest for ISO at given path.
+
+        :param iso_path: (str) absolute path to the ISO
+        """
+        dir, filename = os.path.split(iso_path)
+        iso = IsoWrapper()
+        run("cd %s && %s" % (pipes.quote(dir), iso.get_manifest_cmd(filename)))
+
 
 def get_ks_in(compose, arch, variant):
     data = get_arch_variant_data(compose.conf, "live_images", arch, variant)

tests/test_liveimagesphase.py

@@ -10,26 +10,16 @@ import sys
 
 sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))
 
-from pungi.phases.live_images import LiveImagesPhase
+from pungi.phases.live_images import LiveImagesPhase, CreateLiveImageThread
 from pungi.util import get_arch_variant_data
 
 
 class _DummyCompose(object):
     def __init__(self, config):
-        self.compose_date = '20151203'
+        self.compose_id = 'Test-20151203.0.t'
-        self.compose_type_suffix = '.t'
-        self.compose_respin = 0
-        self.ci_base = mock.Mock(
-            release_id='Test-1.0',
-            release=mock.Mock(
-                short='test',
-                version='1.0',
-            ),
-        )
         self.conf = config
         self.paths = mock.Mock(
             compose=mock.Mock(
-                topdir=mock.Mock(return_value='/a/b'),
                 repository=mock.Mock(
                     side_effect=lambda arch, variant, create_dir=False: os.path.join('/repo', arch, variant.uid)
                 ),
@@ -44,12 +34,6 @@ class _DummyCompose(object):
                     )
                 )
             ),
-            work=mock.Mock(
-                image_build_conf=mock.Mock(
-                    side_effect=lambda variant, image_name, image_type:
-                        '-'.join([variant.uid, image_name, image_type])
-                )
-            ),
             log=mock.Mock(
                 log_file=mock.Mock(return_value='/a/b/log/log_file')
             )
@@ -60,7 +44,6 @@ class _DummyCompose(object):
             'Client': mock.Mock(uid='Client', arches=['amd64']),
             'Everything': mock.Mock(uid='Everything', arches=['x86_64', 'amd64']),
         }
-        self.im = mock.Mock()
         self.log_error = mock.Mock()
         self.get_image_name = mock.Mock(return_value='image-name')
 
@@ -103,8 +86,6 @@ class TestLiveImagesPhase(unittest.TestCase):
         # assert at least one thread was started
         self.assertTrue(phase.pool.add.called)
         self.maxDiff = None
-        cmd = ' && '.join(['cd /iso_dir/amd64/Client',
-                           'isoinfo -R -f -i image-name | grep -v \'/TRANS.TBL$\' | sort >> image-name.manifest'])
         self.assertItemsEqual(phase.pool.queue_put.mock_calls,
                               [mock.call((compose,
                                           {'ks_file': '/path/to/ks_file',
@@ -115,10 +96,7 @@ class TestLiveImagesPhase(unittest.TestCase):
                                                      'http://example.com/repo/',
                                                      '/repo/amd64/Everything'],
                                            'label': '',
-                                           'arch': 'amd64',
                                            'name': None,
-                                           'variant': compose.variants['Client'],
-                                           'cmd': cmd,
                                            'iso_path': '/iso_dir/amd64/Client/image-name',
                                            'version': None,
                                            'specfile': None},
@@ -126,5 +104,124 @@ class TestLiveImagesPhase(unittest.TestCase):
                                           'amd64'))])
 
 
+class TestCreateLiveImageThread(unittest.TestCase):
+
+    @mock.patch('shutil.copy2')
+    @mock.patch('pungi.phases.live_images.run')
+    @mock.patch('pungi.phases.live_images.KojiWrapper')
+    def test_process(self, KojiWrapper, run, copy2):
+        compose = _DummyCompose({'koji_profile': 'koji'})
+        pool = mock.Mock()
+        cmd = {
+            'ks_file': '/path/to/ks_file',
+            'build_arch': 'amd64',
+            'wrapped_rpms_path': '/iso_dir/amd64/Client',
+            'scratch': False,
+            'repos': ['/repo/amd64/Client',
+                      'http://example.com/repo/',
+                      '/repo/amd64/Everything'],
+            'label': '',
+            'name': None,
+            'iso_path': '/iso_dir/amd64/Client/image-name',
+            'version': None,
+            'specfile': None
+        }
+
+        koji_wrapper = KojiWrapper.return_value
+        koji_wrapper.get_create_image_cmd.return_value = 'koji spin-livecd ...'
+        koji_wrapper.run_create_image_cmd.return_value = {
+            'retcode': 0,
+            'output': 'some output',
+            'task_id': 123
+        }
+        koji_wrapper.get_image_path.return_value = ['/path/to/image']
+
+        t = CreateLiveImageThread(pool)
+        with mock.patch('time.sleep'):
+            t.process((compose, cmd, compose.variants['Client'], 'amd64'), 1)
+
+        self.assertEqual(koji_wrapper.run_create_image_cmd.mock_calls,
+                         [mock.call('koji spin-livecd ...', log_file='/a/b/log/log_file')])
+        self.assertEqual(koji_wrapper.get_image_path.mock_calls, [mock.call(123)])
+        self.assertEqual(copy2.mock_calls,
+                         [mock.call('/path/to/image', '/iso_dir/amd64/Client/image-name')])
+
+        write_manifest_cmd = ' && '.join([
+            'cd /iso_dir/amd64/Client',
+            'isoinfo -R -f -i image-name | grep -v \'/TRANS.TBL$\' | sort >> image-name.manifest'
+        ])
+        self.assertEqual(run.mock_calls, [mock.call(write_manifest_cmd)])
+
+    @mock.patch('shutil.copy2')
+    @mock.patch('pungi.phases.live_images.run')
+    @mock.patch('pungi.phases.live_images.KojiWrapper')
+    def test_process_handles_fail(self, KojiWrapper, run, copy2):
+        compose = _DummyCompose({
+            'koji_profile': 'koji',
+            'failable_deliverables': [('^.+$', {'*': ['live']})],
+        })
+        pool = mock.Mock()
+        cmd = {
+            'ks_file': '/path/to/ks_file',
+            'build_arch': 'amd64',
+            'wrapped_rpms_path': '/iso_dir/amd64/Client',
+            'scratch': False,
+            'repos': ['/repo/amd64/Client',
+                      'http://example.com/repo/',
+                      '/repo/amd64/Everything'],
+            'label': '',
+            'name': None,
+            'iso_path': '/iso_dir/amd64/Client/image-name',
+            'version': None,
+            'specfile': None
+        }
+
+        koji_wrapper = KojiWrapper.return_value
+        koji_wrapper.get_create_image_cmd.return_value = 'koji spin-livecd ...'
+        koji_wrapper.run_create_image_cmd.return_value = {
+            'retcode': 1,
+            'output': 'some output',
+            'task_id': 123
+        }
+
+        t = CreateLiveImageThread(pool)
+        with mock.patch('time.sleep'):
+            t.process((compose, cmd, compose.variants['Client'], 'amd64'), 1)
+
+    @mock.patch('shutil.copy2')
+    @mock.patch('pungi.phases.live_images.run')
+    @mock.patch('pungi.phases.live_images.KojiWrapper')
+    def test_process_handles_exception(self, KojiWrapper, run, copy2):
+        compose = _DummyCompose({
+            'koji_profile': 'koji',
+            'failable_deliverables': [('^.+$', {'*': ['live']})],
+        })
+        pool = mock.Mock()
+        cmd = {
+            'ks_file': '/path/to/ks_file',
+            'build_arch': 'amd64',
+            'wrapped_rpms_path': '/iso_dir/amd64/Client',
+            'scratch': False,
+            'repos': ['/repo/amd64/Client',
+                      'http://example.com/repo/',
+                      '/repo/amd64/Everything'],
+            'label': '',
+            'name': None,
+            'iso_path': '/iso_dir/amd64/Client/image-name',
+            'version': None,
+            'specfile': None
+        }
+
+        def boom(*args, **kwargs):
+            raise RuntimeError('BOOM')
+
+        koji_wrapper = KojiWrapper.return_value
+        koji_wrapper.get_create_image_cmd.side_effect = boom
+
+        t = CreateLiveImageThread(pool)
+        with mock.patch('time.sleep'):
+            t.process((compose, cmd, compose.variants['Client'], 'amd64'), 1)
+
+
 if __name__ == "__main__":
     unittest.main()