Code review comment for lp:~adam-collard/landscape-client/juju-info-at-registration

Revision history for this message
Alberto Donato (ack) wrote :

+1, looks good!

#1:
+ juju_registration_info = {}
+ juju_registration_info["environment-uuid"] = juju_info[
+ "environment-uuid"]
+ juju_registration_info["api-addresses"] = juju_info[
+ "api-addresses"].split()
+ juju_registration_info["unit-name"] = juju_info["unit-name"]

I think the juju_info["api-addresses"].split() should me moved inside get_juju_info(), since there are currently two places where the split is performed after getting the info.
As a result this could we written as

+ juju_registration_info = dict(
+ (key, value) for key, value in juju_info
+ if key in ["environment-uuid", "api-addresses", "unit-name"])

#2:
+import os.path

Please use "from os import path" instead.

#3:
+class JujuTest(LandscapeTest):

Tests in this class don't have docstrings.

#4:
+ self.assertEqual("DEAD-BEEF", juju_info["environment-uuid"])
+ self.assertEqual("juju-unit-name", juju_info["unit-name"])
+ self.assertEqual("10.0.3.1:17070", juju_info["api-addresses"])
+ self.assertEqual("127.0.0.1", juju_info["private-address"])

It'd be better to compare juju_info with an expected dict, to ensure there are no extra keys.

review: Approve

« Back to merge proposal