Code review comment for lp:~alexandru-sirbu/cloud-init/bigstep-datasource-improvements

On 07/03/16 12:18, Alex Sirbu wrote:
>> === modified file 'cloudinit/sources/DataSourceBigstep.py'
>> --- cloudinit/sources/DataSourceBigstep.py 2016-03-02 08:53:47 +0000
>> +++ cloudinit/sources/DataSourceBigstep.py 2016-03-07 09:36:05 +0000
>> @@ -22,7 +23,13 @@
>> self.userdata_raw = ""
>>
>> def get_data(self, apply_filter=False):
>> - url = get_url_from_file()
>> + try:
>
> So, have the try catch there and return a NULL value or something similar and then return False if the url returned by the function has that value?

Yeah, I think that makes sense.

>> + url = get_url_from_file()
>> + except IOError as e:
>> + if e.errno == errno.ENOENT:
>
> Exactly, if the file does not exist (the case covered by the ENOENT) then it is ok, as we probably are on a non-Bigstep server. However, if another error is raised, then it is probably worthwhile to see and investigate the issue and is probably not caused by the inexistance of the file. The same error catching is also used in the load_config function in the read_conf from the util file -> the inexistance of the file is treated as an empty config, any other error is thrown anyway. Does an explanation need to be added in this case?

It's not immediately obvious to me, so I would still like one. Just
something like "If the file doesn't exist, this isn't Bigstep;
otherwise, this is broken Bigstep".