Merge lp:~mvo/ubuntu/vivid/ubuntu-core-upgrader/add-test-tar-generator into lp:ubuntu/vivid/ubuntu-core-upgrader

[Michael Vogt]

This branch adds a minimal test for the tar_generator() method. With that
in place the other parts of the function (like removed_files handling etc)
can be tested.

I moved the function it out of "Upgrader" because upgrader takes the "options" argument and that caused me some headache to mock. But it does not have to be this way, if we create a MockOptions. But the advantage of having it as a independent function is that all state is explicitly passed in and we don't have to worry about internal object state.

[James Hunt (jamesodhunt)]

Looks good as a start.

As mentioned, unfortunate that we were both working on the same code at the same time, but I can rework my code again to fit in with what we have here. Note that I had solved the options issue by passing a dict of options to the Upgrader() rather than an argparse Namespace() object.

review: Approve

[Michael Vogt (mvo)]

Yeah, passing a dict or a unittest.mock.Mock() will work just fine to mock the options I just didn't do it for this test but for others I'm sure it will be needed :)

=== added file 'ubuntucoreupgrader/tests/test_upgrader.py'
--- ubuntucoreupgrader/tests/test_upgrader.py	1970-01-01 00:00:00 +0000
+++ ubuntucoreupgrader/tests/test_upgrader.py	2015-02-27 09:50:52 +0000
@@ -0,0 +1,26 @@
+#!/usr/bin/python
+
+import tarfile
+import tempfile
+import unittest
+
+from ubuntucoreupgrader.upgrader import (
+    tar_generator,
+)
+
+
+class UpgradeTestCase(unittest.TestCase):
+
+    def test_tar_generator_regression_unpack_assets(self):
+        tempf = tempfile.TemporaryFile()
+        with tarfile.TarFile(fileobj=tempf, mode="w") as tar:
+            tar.add(__file__, "system/bin/true")
+            tar.add(__file__, "assets/kernel")
+            tar.add(__file__, "unreleated/something")
+
+            result = [m.name for m in tar_generator(tar, "cache_dir", [], "/")]
+            self.assertEqual(result, ["system/bin/true", "assets/kernel"])
+
+
+if __name__ == "__main__":
+    unittest.main()

=== modified file 'ubuntucoreupgrader/upgrader.py'
--- ubuntucoreupgrader/upgrader.py	2015-02-20 17:00:34 +0000
+++ ubuntucoreupgrader/upgrader.py	2015-02-27 09:50:52 +0000
@@ -727,6 +727,89 @@
         return True
 
 
+def tar_generator(tar, cache_dir, removed_files, root_dir):
+    '''
+    Generator function to handle extracting members from the system
+    image tar file.
+    '''
+    for member in tar:
+
+        # Restrictions:
+        #
+        # - Don't unpack the removed file.
+        # - Don't unpack device files *iff* they already exist.
+        # - Don't unpack files that are not below
+        #   TAR_FILE_SYSTEM_PREFIX or TAR_FILE_ASSETS_PREFIX.
+        #
+        # - XXX: This is a temporary function to work-around for
+        #   LP: #1381121: we shouldn't need to filter which files are
+        #   extracted!
+        device_prefix = '{}dev/'.format(TAR_FILE_SYSTEM_PREFIX)
+
+        mount_path = '{}/{}'.format(cache_dir, member.name)
+
+        unpack = True
+
+        if member.name == removed_files:
+            # already handled
+            unpack = False
+
+        if member.name.startswith(device_prefix) and \
+                os.path.exists(mount_path):
+            # device already exists
+            unpack = False
+
+        if not (member.name.startswith(TAR_FILE_SYSTEM_PREFIX) or
+                member.name.startswith(TAR_FILE_ASSETS_PREFIX)):
+            # unrecognised prefix directory
+            unpack = False
+
+        if not unpack:
+            log.debug('not unpacking file {}'.format(member.name))
+        else:
+            # A modified root directory requires convering
+            # absolute paths to be located below the modified root
+            # directory.
+            if root_dir != '/':
+                base = remove_prefix(member.name)
+                member.name = '{}{}'.format(root_dir, base)
+
+                if member.type in (tarfile.LNKTYPE, tarfile.SYMTYPE) \
+                        and member.linkname.startswith('/'):
+                    # Hard and symbolic links also need their
+                    # 'source' updated to take account of the root
+                    # directory.
+                    #
+                    # But rather than remove the prefix, we add the
+                    # root directory as a prefix to contain the link
+                    # within that root.
+                    base = os.path.join(root_dir, member.linkname)
+
+                    member.linkname = '{}{}'.format(root_dir, base)
+            else:
+                path = mount_path
+
+            log.debug('unpacking file {}'.format(member.name))
+
+            # If the file is a binary and that binary is currently
+            # being executed by a process, attempting to unpack it
+            # will result in ETXTBSY (OSError: [Errno 26] Text file
+            # busy). The simplest way around this issue is to unlink
+            # the file just before unpacking it (ideally, we'd catch
+            # the exception and handle it separately). This allows
+            # the unpack to continue, and the running process to
+            # continue to use it's (old) version of the binary until
+            # it's corresponding service is restarted.
+            #
+            # Note that at this point, we already have another copy
+            # of the inode below /lost+found/.
+            if not member.isdir() and os.path.lexists(path):
+                log.debug('removing file {}'.format(path))
+                os.unlink(path)
+
+            yield member
+
+
 class Upgrader():
 
     # FIXME: Should query system-image-cli (see bug LP:#1380574).
@@ -1436,90 +1519,6 @@
                     if e.errno != errno.ENOTEMPTY:
                         raise e
 
-    def tar_generator(self, members):
-        '''
-        Generator function to handle extracting members from the system
-        image tar file.
-        '''
-        for member in members:
-
-            # Restrictions:
-            #
-            # - Don't unpack the removed file.
-            # - Don't unpack device files *iff* they already exist.
-            # - Don't unpack files that are not below
-            #   TAR_FILE_SYSTEM_PREFIX or TAR_FILE_ASSETS_PREFIX.
-            #
-            # - XXX: This is a temporary function to work-around for
-            #   LP: #1381121: we shouldn't need to filter which files are
-            #   extracted!
-            device_prefix = '{}dev/'.format(TAR_FILE_SYSTEM_PREFIX)
-
-            mount_path = '{}/{}'.format(self.get_cache_dir(), member.name)
-
-            unpack = True
-
-            if member.name == self.removed_file:
-                # already handled
-                unpack = False
-
-            if member.name.startswith(device_prefix) and \
-                    os.path.exists(mount_path):
-                # device already exists
-                unpack = False
-
-            if not (member.name.startswith(TAR_FILE_SYSTEM_PREFIX) or
-                    member.name.startswith(TAR_FILE_ASSETS_PREFIX)):
-                # unrecognised prefix directory
-                unpack = False
-
-            if not unpack:
-                log.debug('not unpacking file {}'.format(member.name))
-            else:
-                # A modified root directory requires convering
-                # absolute paths to be located below the modified root
-                # directory.
-                if self.options.root_dir != '/':
-                    base = remove_prefix(member.name)
-                    member.name = '{}{}'.format(self.options.root_dir, base)
-
-                    if member.type in (tarfile.LNKTYPE, tarfile.SYMTYPE) \
-                            and member.linkname.startswith('/'):
-                        # Hard and symbolic links also need their
-                        # 'source' updated to take account of the root
-                        # directory.
-                        #
-                        # But rather than remove the prefix, we add the
-                        # root directory as a prefix to contain the link
-                        # within that root.
-                        base = os.path.join(self.options.root_dir,
-                                            member.linkname)
-
-                        member.linkname = '{}{}'.format(self.options.root_dir,
-                                                        base)
-                else:
-                    path = mount_path
-
-                log.debug('unpacking file {}'.format(member.name))
-
-                # If the file is a binary and that binary is currently
-                # being executed by a process, attempting to unpack it
-                # will result in ETXTBSY (OSError: [Errno 26] Text file
-                # busy). The simplest way around this issue is to unlink
-                # the file just before unpacking it (ideally, we'd catch
-                # the exception and handle it separately). This allows
-                # the unpack to continue, and the running process to
-                # continue to use it's (old) version of the binary until
-                # it's corresponding service is restarted.
-                #
-                # Note that at this point, we already have another copy
-                # of the inode below /lost+found/.
-                if not member.isdir() and os.path.lexists(path):
-                    log.debug('removing file {}'.format(path))
-                    os.unlink(path)
-
-                yield member
-
     def _cmd_update(self, args):
         '''
         Unpack a new system image.
@@ -1667,7 +1666,9 @@
             with patch("grp.getgrnam") as m:
                 m.side_effect = KeyError()
                 tar.extractall(path=self.get_cache_dir(),
-                               members=self.tar_generator(tar))
+                               members=tar_generator(
+                                   tar, self.get_cache_dir(),
+                                   self.removed_file, self.options.root_dir))
         tar.close()
 
         if self.upgrade_type == self.upgrade_types.UPGRADE_AB_PARTITIONS: