cloud-init-dev team mailing list archive
-
cloud-init-dev team
-
Mailing list archive
-
Message #02075
Re: [Merge] ~paul-meyer/cloud-init:bug-1687712 into cloud-init:master
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.
References