Code review comment for lp:~chad.smith/landscape-client/avoid-metadata-pycurl-traceback

Revision history for this message
Chris Glass (tribaal) wrote :

Looks good! +1 with a couple of refactoring/renaming comments:

[1]
I feel the term "cloud" is a little overused everywhere, making things less obvious that they could be. In the future I suppose we will add meta data for more than just EC2, so how about:
- Renaming _fetch_cloud_meta_data to _fetch_and_log_ec2_data (or similar) as this is really an EC2 specific method.
- Changing the logs to say "EC2" instead of "Cloud" in the aforementioned method

[2]
I would like to keep the message content gathering and creation logic in _create_computer_info_message. This would simply mean moving

+ if (self._cloud_meta_data is None and
+ self._cloud_retries < METADATA_RETRY_MAX):
+ self._cloud_meta_data = yield self._fetch_cloud_meta_data()

to _create_computer_info_message, and marking _create_computer_info_message as @inlineCallback instead of send_computer_message

If this is done, I'm not certain it's useful to keep self._cloud_meta_data (maybe a local variable is sufficient inside _create_computer_info_message)

Then in the future we can then easily extend the functionality by adding _fetch_and_log_<whatever>_data() methods to the class and hooking it into _create_computer_info_message.

review: Approve

« Back to merge proposal