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. _______________________________________________ Mailing list: https://launchpad.net/~cloud-init-dev Post to : [email protected] Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
