cloud-init-dev team mailing list archive

[Scott Moser <smoser@xxxxxxxxxx>]

Paul,
Thanks for finding this bug and fixing.

Please don't take my comments as harsh.  Your help is really appreciated.

There are a few small comments inline.


Diff comments:

> diff --git a/cloudinit/config/cc_disk_setup.py b/cloudinit/config/cc_disk_setup.py
> index f49386e..32d51e8 100644
> --- a/cloudinit/config/cc_disk_setup.py
> +++ b/cloudinit/config/cc_disk_setup.py
> @@ -936,14 +938,14 @@ def mkfs(fs_cfg):
>          if overwrite or device_type(device) == "disk":
>              fs_cmd.append(lookup_force_flag(fs_type))
>  
> -    # Add the extends FS options
> -    if fs_opts:
> -        fs_cmd.extend(fs_opts)
> +        # Add the extends FS options
> +        if fs_opts:
> +            fs_cmd.extend(fs_opts)
>  
>      LOG.debug("Creating file system %s on %s", label, device)
> -    LOG.debug("     Using cmd: %s", " ".join(fs_cmd))
> +    LOG.debug("     Using cmd: %s", fs_cmd if shell else " ".join(fs_cmd))

I think this change is confusing.  The subp will log the commadn that is invoked, but this here might make me think we're using a string command when we were more correctly using a array.
maybe just str(fs_cmd) ?

>      try:
> -        util.subp(fs_cmd)
> +        util.subp(fs_cmd, shell=shell)
>      except Exception as e:
>          raise Exception("Failed to exec of '%s':\n%s" % (fs_cmd, e))
>  
> diff --git a/tests/unittests/test_handler/test_handler_disk_setup.py b/tests/unittests/test_handler/test_handler_disk_setup.py
> index 7ff3922..f8e08f3 100644
> --- a/tests/unittests/test_handler/test_handler_disk_setup.py
> +++ b/tests/unittests/test_handler/test_handler_disk_setup.py
> @@ -147,4 +151,51 @@ class TestUpdateFsSetupDevices(TestCase):
>              'filesystem': 'xfs'
>          }, fs_setup)
>  
> +
> +class TestMkfsCommandHandling(TestCase):
> +
> +    def setUp(self):
> +        super(TestMkfsCommandHandling, self).setUp()
> +        self.patches = ExitStack()
> +        mod_name = 'cloudinit.config.cc_disk_setup'
> +        self.find_device_node = self.patches.enter_context(
> +            mock.patch('{0}.find_device_node'.format(mod_name)))
> +        self.find_device_node.return_value = (mock.MagicMock()
> +                                              for _ in range(2))

thank you for adding unit tets.
reading this 'for _ in range(2)' is hard. i'm not sure what it does.

Since you've only got 2 calls, I think you could just as well use the patch decorator on the test cases, and then look at the calls for the mocked subp.

Does that make sense ?

> +
> +        self.subp = self.patches.enter_context(
> +            mock.patch.object(cc_disk_setup.util, 'subp'))
> +
> +        def _subp(cmd, *args, **kwargs):
> +            if kwargs.get('shell'):
> +                self.assertIs(type(cmd), str,
> +                              msg='Expected shell command to be a string')
> +            else:
> +                self.assertIs(type(cmd), list,
> +                              msg='Expected exec command to be a list')
> +            return "success", None
> +        self.subp.side_effect = _subp
> +
> +    def tearDown(self):
> +        super(TestMkfsCommandHandling, self).tearDown()
> +        self.patches.close()
> +
> +    def test_with_cmd(self):
> +        cc_disk_setup.mkfs({
> +            'special': '',
> +            'cmd': 'mkfs -t %(filesystem)s -L %(label)s %(device)s',
> +            'filesystem': 'ext4',
> +            'device': '/dev/xdb1',
> +            'label': 'repro',
> +            'extra_opts': ['should', 'be', 'ignored']

I think the code path should have a warning on this.. if there are extra opts and we're going to render a string.

> +        })
> +
> +    def test_without_cmd(self):
> +        cc_disk_setup.mkfs({
> +            'filesystem': 'ext4',
> +            'device': '/dev/xdb1',
> +            'label': 'repro',
> +            'extra_opts': ['are', 'added']

would be nice to check the subp had these also.

> +        })
> +
>  # vi: ts=4 expandtab


-- 
https://code.launchpad.net/~paul-meyer/cloud-init/+git/cloud-init/+merge/323532
Your team cloud init development team is requested to review the proposed merge of ~paul-meyer/cloud-init:bug-1687712 into cloud-init:master.