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

Nice work Chad, a few other minor points, but looks good. +1!

[4]

+def fetch_ec2_item(path, accumulate, fetch=None):

Since this is an internal helper, not relevant to cloud.py consumers would you please make it private? And move it to the bottom of the file (you can still keep the unit tests and just mark them as wb, e.g. test_wb_fetch_ec2_item_error_returns_failure).

See also:

https://wiki.canonical.com/Landscape/SpecRegistry/0009#Public_and_private

[5]

+def fetch_ec2_meta_data(fetch=None):

Please document the fetch parameter and mention that it's there for testing purposes.

[6]

+ def _unicode_none(value):
+ if value is None:
+ return None
+ else:
+ return value.decode("utf-8")

This could be simply:

        def _unicode_none(value):
            if value is not None:
                return value.decode("utf-8")

I'd also rename it to unicode_or_none (no need to make it private since it's local scope).

[7]

+ logging.info("Acquired cloud meta-data.")

It's generally better to keep logging functionality outside of library functions, this grants a bit more flexibility to the calling code, that can decide what, when and how to log (and indeed computerinfo.py is where the rest of the logging is done). This is minor, so feel free to address it or not.

[8]

+METADATA_RETRY_MAX = 3 # Number of retries to get EC2 meta-data

This should probably be moved to computerinfo.py, since it's where it's used (while cloud.py ignores it).

[9]

+ else:
+ logging.warning(
+ "Temporary failure accessing cloud meta-data, retrying.")

There's no need to log retries I think, especially because non-cloud clients will always get this message.

[10]

+ log_failure(
+ error, msg=(
+ "Max retries reached querying meta-data. %s" %
+ error.getErrorMessage()))

I don't think it should be logged as an error, as this just fine for non-cloud clients. I'd simply use logging.info with a message like "No cloud meta-data available.".