cloud-init-dev team mailing list archive

[Scott Moser <smoser@xxxxxxxxxx>]

clean up the comment i guess.  we can use os.path.basename in both places.


Diff comments:

> diff --git a/cloudinit/sources/DataSourceOVF.py b/cloudinit/sources/DataSourceOVF.py
> index 24b45d5..4b02cbb 100644
> --- a/cloudinit/sources/DataSourceOVF.py
> +++ b/cloudinit/sources/DataSourceOVF.py
> @@ -406,29 +406,19 @@ def transport_iso9660(require_iso=True):
>      else:
>          mtype = None
>  
> -    devs = os.listdir("/dev/")
> -    devs.sort()
> +    # generate a list of devices with mtype filesystem, filter by regex
> +    devs = [dev for dev in
> +            util.find_devs_with("TYPE=%s" % mtype if mtype else None)
> +            if cdmatch.match(dev[5:])]

I guess that is fine.

>      for dev in devs:
> -        fullp = os.path.join("/dev/", dev)
> -
> -        if (fullp in mounts or
> -                not cdmatch.match(dev) or os.path.isdir(fullp)):
> -            continue
> -
>          try:
> -            # See if we can read anything at all...??
> -            util.peek_file(fullp, 512)
> -        except IOError:
> -            continue
> -
> -        try:
> -            (fname, contents) = util.mount_cb(fullp, get_ovf_env, mtype=mtype)
> +            (fname, contents) = util.mount_cb(dev, get_ovf_env, mtype=mtype)
>          except util.MountFailedError:
> -            LOG.debug("%s not mountable as iso9660", fullp)
> +            LOG.debug("%s not mountable as iso9660", dev)
>              continue
>  
>          if contents is not False:
> -            return (contents, fullp, fname)
> +            return (contents, dev, fname)
>  
>      return (False, None, None)
>  
> diff --git a/tests/unittests/test_datasource/test_ovf.py b/tests/unittests/test_datasource/test_ovf.py
> index 9dbf4dd..7c416a5 100644
> --- a/tests/unittests/test_datasource/test_ovf.py
> +++ b/tests/unittests/test_datasource/test_ovf.py
> @@ -70,4 +71,112 @@ class TestReadOvfEnv(test_helpers.TestCase):
>          self.assertEqual({'password': "passw0rd"}, cfg)
>          self.assertIsNone(ud)
>  
> +
> +class TestTransportIso9660(test_helpers.CiTestCase):
> +
> +    def setUp(self):
> +        super(TestTransportIso9660, self).setUp()
> +        self.add_patch('cloudinit.util.find_devs_with',
> +                       'm_find_devs_with')
> +        self.add_patch('cloudinit.util.mounts', 'm_mounts')
> +        self.add_patch('cloudinit.util.mount_cb', 'm_mount_cb')
> +        self.add_patch('cloudinit.sources.DataSourceOVF.get_ovf_env',
> +                       'm_get_ovf_env')
> +        self.m_get_ovf_env.return_value = ('myfile', 'mycontent')
> +
> +    def test_find_already_mounted(self):
> +        """Check we call get_ovf_env from on matching mounted devices"""
> +        mounts = {
> +            '/dev/sr9': {
> +                'fstype': 'iso9660',
> +                'mountpoint': 'wark/media/sr9',
> +                'opts': 'ro',
> +            }
> +        }
> +        self.m_mounts.return_value = mounts
> +
> +        (contents, fullp, fname) = dsovf.transport_iso9660()
> +        self.assertEqual("mycontent", contents)
> +        self.assertEqual("/dev/sr9", fullp)
> +        self.assertEqual("myfile", fname)
> +
> +    def test_find_already_mounted_skips_non_iso9660(self):
> +        """Check we call get_ovf_env ignoring non iso9660"""
> +        mounts = {
> +            '/dev/xvdb': {
> +                'fstype': 'vfat',
> +                'mountpoint': 'wark/foobar',
> +                'opts': 'defaults,noatime',
> +            },
> +            '/dev/xvdc': {
> +                'fstype': 'iso9660',
> +                'mountpoint': 'wark/media/sr9',
> +                'opts': 'ro',
> +            }
> +        }
> +        # ensure we check xvdb before xvdc
> +        self.m_mounts.return_value = (
> +            OrderedDict(sorted(mounts.items(), key=lambda t: t[0])))

ok. it was just the comment that was confusing then.

> +
> +        (contents, fullp, fname) = dsovf.transport_iso9660()
> +        self.assertEqual("mycontent", contents)
> +        self.assertEqual("/dev/xvdc", fullp)
> +        self.assertEqual("myfile", fname)
> +
> +    def test_mount_cb_called_on_blkdevs_with_iso9660(self):
> +        """Check we call mount_cb on blockdevs with iso9660 only"""
> +        self.m_mounts.return_value = {}
> +        self.m_find_devs_with.return_value = ['/dev/sr0']
> +        self.m_mount_cb.return_value = ("myfile", "mycontent")
> +
> +        (contents, fullp, fname) = dsovf.transport_iso9660()
> +
> +        self.m_mount_cb.assert_called_with(
> +            "/dev/sr0", dsovf.get_ovf_env, mtype="iso9660")
> +        self.assertEqual("mycontent", contents)
> +        self.assertEqual("/dev/sr0", fullp)
> +        self.assertEqual("myfile", fname)
> +
> +    def test_mount_cb_called_on_blkdevs_with_iso9660_check_regex(self):
> +        """Check we call mount_cb on blockdevs with iso9660 and match regex"""
> +        self.m_mounts.return_value = {}
> +        self.m_find_devs_with.return_value = [
> +            '/dev/abc', '/dev/my-cdrom', '/dev/sr0']
> +        self.m_mount_cb.return_value = ("myfile", "mycontent")
> +
> +        (contents, fullp, fname) = dsovf.transport_iso9660()
> +
> +        self.m_mount_cb.assert_called_with(
> +            "/dev/sr0", dsovf.get_ovf_env, mtype="iso9660")
> +        self.assertEqual("mycontent", contents)
> +        self.assertEqual("/dev/sr0", fullp)
> +        self.assertEqual("myfile", fname)
> +
> +    def test_mount_cb_not_called_no_matches(self):
> +        """Check we don't call mount_cb if nothing matches"""
> +        self.m_mounts.return_value = {}
> +        self.m_find_devs_with.return_value = ['/dev/vg/myovf']
> +
> +        (contents, fullp, fname) = dsovf.transport_iso9660()
> +
> +        self.assertEqual(0, self.m_mount_cb.call_count)
> +        self.assertEqual(False, contents)
> +        self.assertIsNone(fullp)
> +        self.assertIsNone(fname)
> +
> +    def test_mount_cb_called_require_iso_false(self):
> +        """Check we call mount_cb on blockdevs with require_iso=False"""
> +        self.m_mounts.return_value = {}
> +        self.m_find_devs_with.return_value = ['/dev/xvdz']
> +        self.m_mount_cb.return_value = ("myfile", "mycontent")
> +
> +        (contents, fullp, fname) = dsovf.transport_iso9660(require_iso=False)
> +
> +        self.m_mount_cb.assert_called_with(
> +            "/dev/xvdz", dsovf.get_ovf_env, mtype=None)
> +        self.assertEqual("mycontent", contents)
> +        self.assertEqual("/dev/xvdz", fullp)
> +        self.assertEqual("myfile", fname)
> +
> +
>  # vi: ts=4 expandtab


-- 
https://code.launchpad.net/~raharper/cloud-init/+git/cloud-init/+merge/330995
Your team cloud-init commiters is requested to review the proposed merge of ~raharper/cloud-init:ds-ovf-use-util-find-devs-with into cloud-init:master.