cloud-init-dev team mailing list archive

[Chad Smith <chad.smith@xxxxxxxxxxxxx>]

Diff comments:

> diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py
> index b9458ff..9dc9157 100644
> --- a/cloudinit/sources/DataSourceAzure.py
> +++ b/cloudinit/sources/DataSourceAzure.py
> @@ -319,7 +321,11 @@ class DataSourceAzureNet(sources.DataSource):
>      def get_data(self):
>          # azure removes/ejects the cdrom containing the ovf-env.xml
>          # file on reboot.  So, in order to successfully reboot we
> -        # need to look in the datadir and consider that valid
> +        # need to look in the datadir and consider that valida
> +        asset_tag = util.read_dmi_data('chassis-asset-tag')
> +        if asset_tag != AZURE_CHASSIS_ASSET_TAG:
> +            LOG.info("Non-Azure DMI asset tag '%s' discovered.", asset_tag)

+1 on the noise, I do have a hard time following logs for cloud-init. Adding cloud init logs to the sprint topics.

> +            return False
>          ddir = self.ds_cfg['data_dir']
>  
>          candidates = [self.seed_dir]
> @@ -817,7 +823,8 @@ def load_azure_ds_dir(source_dir):
>      ovf_file = os.path.join(source_dir, "ovf-env.xml")
>  
>      if not os.path.isfile(ovf_file):
> -        raise NonAzureDataSource("No ovf-env file found")
> +        raise NonAzureDataSource(
> +            "No ovf-env.xml file found at %s" % (source_dir))

On second glance we aren't really doing anything with that raise exception message anyway. We just continue through a loop so it's probably not worth the change in the first place. I'll back that out.

>  
>      with open(ovf_file, "rb") as fp:
>          contents = fp.read()
> diff --git a/tests/unittests/test_datasource/test_azure.py b/tests/unittests/test_datasource/test_azure.py
> index 852ec70..f42d9c2 100644
> --- a/tests/unittests/test_datasource/test_azure.py
> +++ b/tests/unittests/test_datasource/test_azure.py
> @@ -241,6 +249,23 @@ fdescfs            /dev/fd          fdescfs rw              0 0
>              res = get_path_dev_freebsd('/etc', mnt_list)
>              self.assertIsNotNone(res)
>  
> +    @mock.patch('cloudinit.sources.DataSourceAzure.util.read_dmi_data')
> +    def test_non_azure_dmi_chassis_asset_tag(self, m_read_dmi_data):
> +        """Report non-azure when DMI's chassis asset tag doesn't match.
> +
> +        Return False when the asset tag doesn't match Azure's static
> +        AZURE_CHASSIS_ASSET_TAG.
> +        """
> +        # Return a non-matching asset tag value
> +        nonazure_tag = dsaz.AZURE_CHASSIS_ASSET_TAG + 'X'
> +        m_read_dmi_data.return_value = nonazure_tag
> +        dsrc = dsaz.DataSourceAzureNet(
> +            {}, distro=None, paths=self.paths)
> +        self.assertFalse(dsrc.get_data())
> +        self.assertEqual(

I wanted this to be specific in this case to just show that we immediately exited without calling other actions which happen to log content. Generally yes, I'd agree we want assertIns for logs so we don't have to maintain these unrelated tests if there are more logs added for in subsequent code runs. Since this was the early exit point, I'd wanted it to fail if other logs were generated.

> +            "Non-Azure DMI asset tag '{0}' discovered.\n".format(nonazure_tag),
> +            self.logs.getvalue())
> +
>      def test_basic_seed_dir(self):
>          odata = {'HostName': "myhost", 'UserName': "myuser"}
>          data = {'ovfcontent': construct_valid_ovf_env(data=odata),
> @@ -696,6 +721,33 @@ class TestAzureBounce(TestCase):
>          self.assertEqual(0, self.set_hostname.call_count)
>  
>  
> +class TestLoadAzureDsDir(CiTestCase):
> +    """Tests for load_azure_ds_dir."""
> +
> +    def setUp(self):
> +        self.source_dir = self.tmp_dir()
> +        super(TestLoadAzureDsDir, self).setUp()
> +
> +    def test_missing_ovf_env_xml_raises_non_azure_datasource_error(self):
> +        """load_azure_ds_dir raises an error When ovf-env.xml doesn't exit."""
> +        with self.assertRaises(dsaz.NonAzureDataSource) as context_manager:
> +            dsaz.load_azure_ds_dir(self.source_dir)
> +        self.assertEqual(
> +            'No ovf-env.xml file found at {0}'.format(self.source_dir),
> +            str(context_manager.exception))
> +
> +    def test_wb_invalid_ovf_env_xml_calls_read_azure_ovf(self):
> +        """load_azure_ds_dir calls read_azure_ovf to parse the xml."""
> +        ovf_path = os.path.join(self.source_dir, 'ovf-env.xml')
> +        with open(ovf_path, 'wb') as stream:
> +            stream.write('invalid xml')
> +        with self.assertRaises(dsaz.BrokenAzureDataSource) as context_manager:
> +            dsaz.load_azure_ds_dir(self.source_dir)
> +        self.assertEqual(
> +            'Invalid ovf-env.xml: syntax error: line 1, column 0',

True, I was trying to assert that we are actually calling read_azure_ovf method without mocks, and BrokenAzureDataSource is only raised fromt read_azure_ovf.  I marked the test as test_wb(whitebox) as it implies knowledge about the internals of an unrelated function. I don't really know what we should do here. I'd like to cover the assertion that we call read_azure_ovf, but I really try to steer away from mocks if at all possible. Thoughts on the approach I should take? I guess both mocking and white box testing tightly couple this unit test to the read_azure_ovf implementation.

> +            str(context_manager.exception))
> +
> +
>  class TestReadAzureOvf(TestCase):
>      def test_invalid_xml_raises_non_azure_ds(self):
>          invalid_xml = "<foo>" + construct_valid_ovf_env(data={})
> diff --git a/tests/unittests/test_ds_identify.py b/tests/unittests/test_ds_identify.py
> index 5c26e65..31335ac 100644
> --- a/tests/unittests/test_ds_identify.py
> +++ b/tests/unittests/test_ds_identify.py
> @@ -268,9 +280,24 @@ def _print_run_output(rc, out, err, cfg, files):
>  
>  
>  VALID_CFG = {
> +<<<<<<< tests/unittests/test_ds_identify.py
>      'AliYun': {
>          'ds': 'AliYun',
>          'files': {P_PRODUCT_NAME: 'Alibaba Cloud ECS\n'},
> +=======

rebased

> +    'Azure-dmi-detection': {
> +        'ds': 'Azure',
> +        'files': {
> +            P_CHASSIS_ASSET_TAG: '7783-7084-3265-9085-8269-3286-77\n',
> +        }
> +    },
> +    'Azure-seed-detection': {
> +        'ds': 'Azure',
> +        'files': {
> +            P_CHASSIS_ASSET_TAG: 'No-match\n',
> +            os.path.join(P_SEED_DIR, 'azure', 'ovf-env.xml'): 'present\n',
> +        }
> +>>>>>>> tests/unittests/test_ds_identify.py
>      },
>      'Ec2-hvm': {
>          'ds': 'Ec2',
> diff --git a/tools/ds-identify b/tools/ds-identify
> index 5fc500b..12633c7 100755
> --- a/tools/ds-identify
> +++ b/tools/ds-identify
> @@ -402,11 +417,6 @@ dmi_product_serial_matches() {
>      return 1
>  }
>  

Nice thorough followup on the review. Very good to know, I ran scripts locally to just see that match behaved like dmi_product_name_is in the altered callsites but didn't think to time the runs. If we can drop dmi_product_name_is it just represents one less function to maintain in my mind,

> -dmi_product_name_is() {
> -    is_container && return 1
> -    [ "${DI_DMI_PRODUCT_NAME}" = "$1" ]
> -}
> -
>  dmi_sys_vendor_is() {
>      is_container && return 1
>      [ "${DI_DMI_SYS_VENDOR}" = "$1" ]


-- 
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/324875
Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:azure-di-id-asset-tag into cloud-init:master.