cloud-init-dev team mailing list archive

[Chad Smith <chad.smith@xxxxxxxxxxxxx>]

Review: Approve

Thanks for this good work approved pending your decision on the following comments. 


My comments will be made per the consolidated visual diff @ http://paste.ubuntu.com/24716726/

line 49 in the above diff:
 Since order of datasources matters to determine which datasource is detectred first in both cloudinit/settings.py CFG_BUILTIN and tools/ds-identify DI_DSLIST_DEFAULT.  Why are these two lists not ordered the same? AliYun is placed just before Ec2 in ds-identify but way at the beginning settings.py. Should we make an attempt to ensure both lists are consistently ordered?


line 95:
- Please add a comment in the code about the intent of NO_EC2_METADATA in  in cloudinit/sources/DataSourceEc2.py as you did in the commit message. The other devs looking at the code for a subclass of DataSourceEc2 could know how and why to make use of this value. commit messages are lost, comments live forever :).


line 103 cloudinit/sources/DataSourceAliYun.py:
- s/NO_EC2_MD/NO_EC2_METADATA/ let's reduce two letter acronyms throughout the code as it frequently involves a second glance to figure out what the intent of the variable is to determine context.

line 104 cloudinit/sources/DataSourceAliYun.py:
I see no unit test coverage of "elif self.cloud_platform == Platforms.NO_EC2_MD:"


line 124 tests/unittests/test_datasource/test_aliyun.py: 
   Since we have mocked _is_aliyun we aren't really testing anywhere that we are getting this info from dmi. It might be better to mock util.read_dmi_data as that is the interface we are calling into. Then we can validate the calls into that interface as follows:

    @mock.patch("cloudinit.util.read_dmi_data")
    @httpretty.activate
    def test_with_mock_server(self, m_read_dmi):
        m_read_dmi.return_value = ay.ALIYUN_PRODUCT 
        self.regist_default_server()
        self.ds.get_data()
        self._test_get_data()
        self._test_get_sshkey()
        self._test_get_iid()
        self._test_host_name()
        m_read_dmi.assert_called_with('system-product-name')

 

line 140 tests/unittests/test_datasource/test_common.py
   These network tests feel like they aren't all testing the right things. Why is the test 
   Why is test_expected_nondefault_network_sources_found still passing or needed? I was expecting to see that AliYun is dropped from nondefault sources.


line 175:  Since DataSourceAliYum.ALIYUN_PRODUCT is already defined in the module, we should use that variable in unit tests instead VALID_CFGs instead of re-typing the string so there is only one source of truth for this data. It also aids in grepping source to find out where ALIYUN_PRODUCT is used throughout code and unit tests.



-- 
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324625
Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:feature/enable-aliyun into cloud-init:master.