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

[Adam Collard]

Add Juju information at registration time.

 * Changed schema of JUJU_INFO message to be strict about the keys
 * Re-used schema for REGISTER and REGISTER_CLOUD_VM messages
 * Added l.lib.juju as resting ground for json parsing and error handling
 * Moved juju_filename to main config object, not just specific to monitor

Add Juju information at registration time.

 * Changed schema of JUJU_INFO message to be strict about the keys
 * Re-used schema for REGISTER and REGISTER_CLOUD_VM messages
 * Added l.lib.juju as resting ground for json parsing and error handling
 * Moved juju_filename to main config object, not just specific to monitor

[Alberto Donato (ack)]

+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

[Björn Tillenius (bjornt)]

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.

review: Needs Fixing

[Adam Collard (adam-collard)]

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.

[Björn Tillenius (bjornt)]

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?

review: Approve

[lp:~adam-collard/landscape-client/juju-info-at-registration]

732. By Björn Tillenius on 2013-09-27

Merge unmet-dependencies-bug-1203855 [f=1203855] [r=tribaal,ack] [a=Björn Tillenius]
Don't omit the package from the package changer result text if it's
broken but we don't know why.

When a package is broken we check certain dependency to see whether they
are satisfied. If all the dependencies we check are satisified, the
package wasn't reported as being broken.

Ideally we should write why the package is broken, but I have been
unable to reproduce the error in the bug report.

733. By Adam Collard on 2013-09-27

Merge juju-info-at-registration [f=1229630] [r=bjornt,ack] [a=Adam Collard]
Add Juju information at registration time.

 * Changed schema of JUJU_INFO message to be strict about the keys
 * Re-used schema for REGISTER and REGISTER_CLOUD_VM messages
 * Added l.lib.juju as resting ground for json parsing and error handling
 * Moved juju_filename to main config object, not just specific to monitor