cloud-init-dev team mailing list archive

[Scott Moser <smoser@xxxxxxxxxx>]

> 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?

I think it probably makes sense to put AliYun at the end on both lists
at this point.  I will make that change.

the list in ds_identify is only used in the case that there is no explicit
list defined in /etc/cloud/cloud.cfg (or cfg.d/*).  So its not that common
of a case that this list is referenced (at least not in Ubuntu where the
package contains a list of datasources that can be configured with
dpkg-reconfigure).

The list that is in ds-identify right now is kind of done over iterative
development when i first worked on it.  It started out as a list
where I had definitive sources at the front and network datasources
at the back ('Ec2', 'GCE'...).

I believe I put 'Aliyun' before 'Ec2' because I was thinking that Ec2 might
end up claiming 'found' on Aliyun cloud.  That is now not the case unless
Aliyun starts crafting dmi data that looks like Ec2... which would break
themselves.

I think we can/should put some more thought into how this list (and that in
settings.py) is maintained.  Once 'strict_id' is turned on in EC2 by default
we will really be able to re-order almost at whim.

> 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
> :).

Well, I sort of agree. (I read 'git log' and 'git blame' a lot, and expect
that other developers do also.  I don't take the time to write useful
commit messages for no reason).

> 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.

OK.

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

i'll try to put something together.

> 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()

It feels less tied to implementation to have the helper in test_aliyun.py.
I should have made an assert that that method was called though.

I can add a test for '_is_aliyun' that mocks read_dmi_data.

> 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.
> 

Well, we should probably just drop that test I guess, and add
Aliyun to the DEFAULT_NETWORK list.

The test_expected_nondefault_network_sources_found test basically is
just testing that the Datasource would be used if a configured list.

I'm not sure why 
 test_expected_default_network_sources_found
is passing though.  I'll investigate.

> 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.

Should we?
I think its arguable in both ways.
Having the value listed and used in the test means that the test is asserting
that the value is valid.
If we have the test just use the value that is in the code, then we're
just asserting that the code is consistent.  It could quite possibly be
consistently wrong.

Simplified example:

 MY_NAME = "foo"

 def get_name():
   return MY_NAME

 def test_get_name_is_foo():
   self.assertEqual(MY_NAME, get_name())

 def test_get_name_is_foo2():
   self.assertEqual("foo", get_name())

If someone changes 'MY_NAME' in the code to be 'foobar', then 
test_get_name_is_foo goes on passing, when it should not, but
test_get_name_is_foo2 catches that breakage.


-- 
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.