Merge lp://staging/~adam-collard/landscape-client/juju-info-at-registration into lp://staging/~landscape/landscape-client/trunk

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

Looks good, nothing major, but still marking it as Needs Fixing,
since [6] might change the diff a bit.

[1]

29 + self._juju_data = self._get_juju_data()

I think it would be cleaner to collect the data when on "run", just
like ec2 data is collected.

[2]

222 + self.assertMessages(self.mstore.get_pending_messages(),
223 + [{"type": "register",
224 + "computer_title": self.config.computer_title,
225 + "account_name": "account_name",
226 + "registration_password": None,
227 + "hostname": socket.getfqdn(),
228 + "vm-info": get_vm_info(),
229 + "tags": None,
230 + "juju-info": {
231 + "environment-uuid": "DEAD-BEEF",
232 + "api-addresses": ["10.0.3.1:17070"],
233 + "unit-name": "juju-unit-name"}
234 + }])

The indentation looks off here. Also, please use valid unit names,
like service/0.

[3]
264 - "tags": None}])
265 + "tags": None,
266 + "juju-info": {
267 + "environment-uuid": "DEAD-BEEF",
268 + "api-addresses": ["10.0.3.1:17070"],
269 + "unit-name": "juju-unit-name"}}])

Indentation is off here as well.

[4]
365 + def test_get_juju_info_sample_data(self):
366 + """L{get_juju_info} works with sample data."""

What does "works with sample data" mean? It's implied that our
code works. Better to explain what the code is expected to do instead.

[5]
384 + self.assertTrue(
385 + "Error attempting to read JSON" in self.logfile.getvalue())

Please use assertIn instead, which gives a better error message on failures.

[6]

435 + "juju-info": KeyDict(juju_data)},
436 + optional=["tags", "vm-info", "public_ipv4", "local_ipv4",
437 + "juju-info"])

I think we shouldn't include juju-info in the register-cloud-vm message.
By default Juju instances won't use that the register-cloud-vm message,
and the plan is to remove it altogether.

On 27 September 2013 15:32, Björn Tillenius <email address hidden> wrote:

> [1]
>
> 29 + self._juju_data = self._get_juju_data()
>
> I think it would be cleaner to collect the data when on "run", just
> like ec2 data is collected.
>

OK. Done!

>
> [2]
>
>
> 222 + self.assertMessages(self.mstore.get_pending_messages(),
> 223 + [{"type": "register",
> 224 + "computer_title":
> self.config.computer_title,
> 225 + "account_name": "account_name",
> 226 + "registration_password": None,
> 227 + "hostname": socket.getfqdn(),
> 228 + "vm-info": get_vm_info(),
> 229 + "tags": None,
> 230 + "juju-info": {
> 231 + "environment-uuid": "DEAD-BEEF",
> 232 + "api-addresses": ["10.0.3.1:17070"],
> 233 + "unit-name": "juju-unit-name"}
> 234 + }])
>
> The indentation looks off here. Also, please use valid unit names,
> like service/0.
>

Emacs wanted to indent it like that - have shifted things around a bit.

Done!

>
>
> [3]
> 264 - "tags": None}])
> 265 + "tags": None,
> 266 + "juju-info": {
> 267 + "environment-uuid": "DEAD-BEEF",
> 268 + "api-addresses": ["10.0.3.1:17070
> "],
> 269 + "unit-name": "juju-unit-name"}}])
>
> Indentation is off here as well.
>

This became irrelevant because of [6]

>
> [4]
> 365 + def test_get_juju_info_sample_data(self):
> 366 + """L{get_juju_info} works with sample data."""
>
> What does "works with sample data" mean? It's implied that our
> code works. Better to explain what the code is expected to do instead.
>

I've tried re-phrasing it. It really is just "does it work with realistic
data"

>
> [5]
> 384 + self.assertTrue(
> 385 + "Error attempting to read JSON" in
> self.logfile.getvalue())
>
> Please use assertIn instead, which gives a better error message on
> failures.
>

Argh. I thought we didn't have it because client has to work on 2.6, but
mocker implements it. Grr, I had it as assertIn and the other test using
assertIs then carefully undid them. Thanks :)

>
>
> [6]
>
> 435 + "juju-info": KeyDict(juju_data)},
> 436 + optional=["tags", "vm-info", "public_ipv4", "local_ipv4",
> 437 + "juju-info"])
>
> I think we shouldn't include juju-info in the register-cloud-vm message.
> By default Juju instances won't use that the register-cloud-vm message,
> and the plan is to remove it altogether.
>

OK, I wasn't sure what this was so erred on the side of caution and
included it. Removed.

Thanks, looks good now, +1!

[7]

358 + def test_get_juju_info_sample_data(self):
359 + """L{get_juju_info} parses realistic data properly."""

I might be nitpicking a bit :), but I'd say never use words like
"works", "properly", "as expected", etc. in test docstrings. How about?

  """ L{get_juju_info} parses JSON data from the juju_filename file."""

[8]

I don't see a test for when get_juju_info parses a file having
multiple API endpoints. Is there one?