← Back to team overview

cloud-init-dev team mailing list archive

Re: [Merge] ~smoser/cloud-init:feature/enable-aliyun into cloud-init:master


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:

    def test_with_mock_server(self, m_read_dmi):
        m_read_dmi.return_value = ay.ALIYUN_PRODUCT 


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.

Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:feature/enable-aliyun into cloud-init:master.

Follow ups