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

=== modified file 'landscape/broker/registration.py'
--- landscape/broker/registration.py	2013-07-09 15:58:36 +0000
+++ landscape/broker/registration.py	2013-09-27 14:18:49 +0000
@@ -5,7 +5,7 @@
 as a new client without providing any identification credentials, and the
 server replies with the available registration mechanisms. At this point
 the machinery in this module will notice that we have no identification
-credentials yet and that the server accepts regiration messages, so it
+credentials yet and that the server accepts registration messages, so it
 will craft an appropriate one and send it out.
 """
 import time
@@ -14,8 +14,11 @@
 
 from twisted.internet.defer import Deferred
 
+from landscape.message_schemas import juju_data
+
 from landscape.lib.bpickle import loads
 from landscape.lib.log import log_failure
+from landscape.lib.juju import get_juju_info
 from landscape.lib.fetch import fetch, FetchError
 from landscape.lib.tag import is_valid_tag_list
 from landscape.lib.network import get_fqdn
@@ -97,6 +100,7 @@
         self._pinger = pinger
         self._message_store = message_store
         self._reactor.call_on("run", self._fetch_ec2_data)
+        self._reactor.call_on("run", self._get_juju_data)
         self._reactor.call_on("pre-exchange", self._handle_pre_exchange)
         self._reactor.call_on("exchange-done", self._handle_exchange_done)
         self._exchange.register_message("set-id", self._handle_set_id)
@@ -107,6 +111,7 @@
         self._fetch_async = fetch_async
         self._otp = None
         self._ec2_data = None
+        self._juju_data = None
 
     def should_register(self):
         id = self._identity
@@ -134,6 +139,14 @@
         self._exchange.exchange()
         return result
 
+    def _get_juju_data(self):
+        """Load Juju information."""
+        juju_info = get_juju_info(self._config)
+        if juju_info is None:
+            return None
+        self._juju_data = dict(
+            (key, juju_info[key]) for key in juju_data)
+
     def _get_data(self, path, accumulate):
         """
         Get data at C{path} on the EC2 API endpoint, and add the result to the
@@ -254,65 +267,70 @@
         # registration.
         self._should_register = self.should_register()
 
-        if self._should_register:
-            id = self._identity
-            self._message_store.delete_all_messages()
-            tags = id.tags
-            if not is_valid_tag_list(tags):
-                tags = None
-                logging.error("Invalid tags provided for cloud "
-                              "registration.")
-            if self._config.cloud and self._ec2_data is not None:
-                if self._otp:
-                    logging.info("Queueing message to register with OTP")
-                    message = {"type": "register-cloud-vm",
-                               "otp": self._otp,
-                               "hostname": get_fqdn(),
-                               "account_name": None,
-                               "registration_password": None,
-                               "tags": tags,
-                               "vm-info": get_vm_info()}
-                    message.update(self._ec2_data)
-                    self._exchange.send(message)
-                elif id.account_name:
-                    with_tags = ["", u"and tags %s " % tags][bool(tags)]
-                    logging.info(
-                        u"Queueing message to register with account %r %s"
-                        u"as an EC2 instance." % (id.account_name, with_tags))
-                    message = {"type": "register-cloud-vm",
-                               "otp": None,
-                               "hostname": get_fqdn(),
-                               "account_name": id.account_name,
-                               "registration_password": \
-                                   id.registration_key,
-                               "tags": tags,
-                               "vm-info": get_vm_info()}
-                    message.update(self._ec2_data)
-                    self._exchange.send(message)
-                else:
-                    self._reactor.fire("registration-failed")
+        if not self._should_register:
+            return
+        id = self._identity
+        self._message_store.delete_all_messages()
+        tags = id.tags
+        if not is_valid_tag_list(tags):
+            tags = None
+            logging.error("Invalid tags provided for cloud registration.")
+        if self._config.cloud and self._ec2_data is not None:
+            if self._otp:
+                logging.info("Queueing message to register with OTP")
+                message = {"type": "register-cloud-vm",
+                           "otp": self._otp,
+                           "hostname": get_fqdn(),
+                           "account_name": None,
+                           "registration_password": None,
+                           "tags": tags,
+                           "vm-info": get_vm_info()}
+                message.update(self._ec2_data)
+                if self._juju_data is not None:
+                    message["juju-info"] = self._juju_data
+                self._exchange.send(message)
             elif id.account_name:
-                with_word = ["without", "with"][bool(id.registration_key)]
                 with_tags = ["", u"and tags %s " % tags][bool(tags)]
-                logging.info(u"Queueing message to register with account %r %s"
-                             "%s a password." % (id.account_name, with_tags,
-                                                 with_word))
-                message = {"type": "register",
-                           "computer_title": id.computer_title,
+                logging.info(
+                    u"Queueing message to register with account %r %s"
+                    u"as an EC2 instance." % (id.account_name, with_tags))
+                message = {"type": "register-cloud-vm",
+                           "otp": None,
+                           "hostname": get_fqdn(),
                            "account_name": id.account_name,
                            "registration_password": id.registration_key,
-                           "hostname": get_fqdn(),
                            "tags": tags,
                            "vm-info": get_vm_info()}
-                self._exchange.send(message)
-            elif self._config.provisioning_otp:
-                logging.info(u"Queueing message to register with OTP as a"
-                             u" newly provisioned machine.")
-                message = {"type": "register-provisioned-machine",
-                           "otp": self._config.provisioning_otp}
+                message.update(self._ec2_data)
+                if self._juju_data is not None:
+                    message["juju-info"] = self._juju_data
                 self._exchange.send(message)
             else:
                 self._reactor.fire("registration-failed")
+        elif id.account_name:
+            with_word = ["without", "with"][bool(id.registration_key)]
+            with_tags = ["", u"and tags %s " % tags][bool(tags)]
+            logging.info(u"Queueing message to register with account %r %s"
+                         "%s a password." % (id.account_name, with_tags,
+                                             with_word))
+            message = {"type": "register",
+                       "computer_title": id.computer_title,
+                       "account_name": id.account_name,
+                       "registration_password": id.registration_key,
+                       "hostname": get_fqdn(),
+                       "tags": tags,
+                       "vm-info": get_vm_info()}
+            if self._juju_data is not None:
+                message["juju-info"] = self._juju_data
+            self._exchange.send(message)
+        elif self._config.provisioning_otp:
+            logging.info(u"Queueing message to register with OTP as a"
+                         u" newly provisioned machine.")
+            message = {"type": "register-provisioned-machine",
+                       "otp": self._config.provisioning_otp}
+            self._exchange.send(message)
+        else:
+            self._reactor.fire("registration-failed")
 
     def _handle_set_id(self, message):
         """Registered handler for the C{"set-id"} event.
 
=== modified file 'landscape/broker/tests/helpers.py'
--- landscape/broker/tests/helpers.py	2013-05-07 22:47:06 +0000
+++ landscape/broker/tests/helpers.py	2013-09-27 14:18:49 +0000
@@ -127,6 +127,9 @@
 
         test_case.fetch_func = fetch_async
         test_case.config.cloud = getattr(test_case, "cloud", False)
+        if hasattr(test_case, "juju_contents"):
+            test_case.makeFile(
+                test_case.juju_contents, path=test_case.config.juju_filename)
         test_case.handler = RegistrationHandler(
             test_case.config, test_case.identity, test_case.reactor,
             test_case.exchanger, test_case.pinger, test_case.mstore,
 
=== modified file 'landscape/broker/tests/test_registration.py'
--- landscape/broker/tests/test_registration.py	2013-08-20 23:48:23 +0000
+++ landscape/broker/tests/test_registration.py	2013-09-27 14:18:49 +0000
@@ -1,3 +1,4 @@
+import json
 import os
 import logging
 import pycurl
@@ -177,7 +178,7 @@
         """
         When a computer_title and account_name are available, no
         secure_id is set, and an exchange is about to happen,
-        queue a registration message.
+        queue a registration message with VM information.
         """
         get_vm_info_mock = self.mocker.replace(get_vm_info)
         get_vm_info_mock()
@@ -484,6 +485,38 @@
                               "tags": None}])
 
 
+class JujuRegistrationHandlerTest(RegistrationHandlerTestBase):
+
+    juju_contents = json.dumps({"environment-uuid": "DEAD-BEEF",
+                                "unit-name": "service/0",
+                                "api-addresses": "10.0.3.1:17070",
+                                "private-address": "127.0.0.1"})
+
+    def test_juju_information_added_when_present(self):
+        """
+        When Juju information is found in $data_dir/juju-info.json,
+        key parts of it are sent in the registration message.
+        """
+        self.mstore.set_accepted_types(["register"])
+        self.config.account_name = "account_name"
+        self.reactor.fire("run")
+        self.reactor.fire("pre-exchange")
+
+        self.assertMessages(
+            self.mstore.get_pending_messages(),
+            [{"type": "register",
+              "computer_title": self.config.computer_title,
+              "account_name": "account_name",
+              "registration_password": None,
+              "hostname": socket.getfqdn(),
+              "vm-info": get_vm_info(),
+              "tags": None,
+              "juju-info": {"environment-uuid": "DEAD-BEEF",
+                            "api-addresses": ["10.0.3.1:17070"],
+                            "unit-name": "service/0"}
+              }])
+
+
 class CloudRegistrationHandlerTest(RegistrationHandlerTestBase):
 
     cloud = True
@@ -574,7 +607,6 @@
                        local_ipv4=u"10.0.0.1",
                        public_ipv4=u"10.0.0.2",
                        tags=None)
-
         # The get_vm_info() needs to be deferred to the else.  If vm-info is
         # specified in kwargs, get_vm_info() will typically be mocked.
         if "vm_info" in kwargs:
@@ -912,7 +944,8 @@
                               "registration_password": u"password",
                               "hostname": socket.getfqdn(),
                               "vm-info": get_vm_info(),
-                              "tags": None}])
+                              "tags": None,
+                              }])
 
     def test_should_register_in_cloud(self):
         """
@@ -1157,7 +1190,5 @@
         self.config.provisioning_otp = ""
         self.reactor.fire("pre-exchange")
 
-        self.assertMessages([],
-                            self.mstore.get_pending_messages())
-        self.assertEqual(u"",
-                         self.logfile.getvalue().strip())
+        self.assertMessages([], self.mstore.get_pending_messages())
+        self.assertEqual(u"", self.logfile.getvalue().strip())
 
=== modified file 'landscape/deployment.py'
--- landscape/deployment.py	2013-09-25 08:50:25 +0000
+++ landscape/deployment.py	2013-09-27 14:18:49 +0000
@@ -419,6 +419,11 @@
         """
         return os.path.join(self.data_path, "annotations.d")
 
+    @property
+    def juju_filename(self):
+        """Get the path to the Juju JSON file."""
+        return os.path.join(self.data_path, "juju-info.json")
+
 
 def get_versioned_persist(service):
     """Get a L{Persist} database with upgrade rules applied.
 
=== added file 'landscape/lib/juju.py'
--- landscape/lib/juju.py	1970-01-01 00:00:00 +0000
+++ landscape/lib/juju.py	2013-09-27 14:18:49 +0000
@@ -0,0 +1,26 @@
+import json
+import logging
+import os.path
+
+from landscape.lib.fs import read_file
+
+
+def get_juju_info(config):
+    """
+    Returns the Juju info or C{None} if the path referenced from
+    L{config} is not a file or that file isn't valid JSON.
+    """
+    juju_filename = config.juju_filename
+    if not os.path.isfile(juju_filename):
+        return None
+    json_contents = read_file(juju_filename)
+    try:
+        juju_info = json.loads(json_contents)
+    except Exception:
+        logging.exception(
+            "Error attempting to read JSON from %s" % juju_filename)
+        return None
+    else:
+        if "api-addresses" in juju_info:
+            juju_info["api-addresses"] = juju_info["api-addresses"].split()
+        return juju_info
 
=== modified file 'landscape/lib/tests/test_encoding.py'
--- landscape/lib/tests/test_encoding.py	2013-03-20 08:58:22 +0000
+++ landscape/lib/tests/test_encoding.py	2013-09-27 14:18:49 +0000
@@ -4,7 +4,7 @@
 from landscape.lib.encoding import encode_if_needed, encode_dict_if_needed
 
 
-class EncodingTestTest(LandscapeTest):
+class EncodingTest(LandscapeTest):
 
     def test_encode_if_needed_utf_string(self):
         """
 
=== added file 'landscape/lib/tests/test_juju.py'
--- landscape/lib/tests/test_juju.py	1970-01-01 00:00:00 +0000
+++ landscape/lib/tests/test_juju.py	2013-09-27 14:18:49 +0000
@@ -0,0 +1,62 @@
+from collections import namedtuple
+import json
+
+from landscape.tests.helpers import LandscapeTest
+from landscape.lib.juju import get_juju_info
+
+
+SAMPLE_JUJU_INFO = json.dumps({"environment-uuid": "DEAD-BEEF",
+                               "unit-name": "service/0",
+                               "api-addresses": "10.0.3.1:17070",
+                               "private-address": "127.0.0.1"})
+
+
+class JujuTest(LandscapeTest):
+
+    Config = namedtuple("Config", "juju_filename")
+
+    def test_get_juju_info_sample_data(self):
+        """L{get_juju_info} parses JSON data from the juju_filename file."""
+        stub_config = self.Config(self.makeFile(SAMPLE_JUJU_INFO))
+        juju_info = get_juju_info(stub_config)
+        self.assertEqual(
+            {u"environment-uuid": "DEAD-BEEF",
+             u"unit-name": "service/0",
+             u"api-addresses": ["10.0.3.1:17070"],
+             u"private-address": "127.0.0.1"}, juju_info)
+
+    def test_get_juju_info_empty_file(self):
+        """
+        If L{get_juju_info} is called with a configuration pointing to
+        an empty file, it returns C{None}.
+        """
+        stub_config = self.Config(self.makeFile(""))
+        juju_info = get_juju_info(stub_config)
+        self.log_helper.ignore_errors(ValueError)
+        self.assertEqual(juju_info, None)
+        self.assertIn("Error attempting to read JSON", self.logfile.getvalue())
+
+    def test_get_juju_info_not_a_file(self):
+        """
+        If L{get_juju_info} is called with a configuration pointing to
+        a directory, it returns C{None}.
+        """
+        stub_config = self.Config("/")
+        juju_info = get_juju_info(stub_config)
+        self.assertIs(juju_info, None)
+
+    def test_get_juju_info_multiple_endpoints(self):
+        """L{get_juju_info} turns space separated API addresses into a list."""
+        juju_multiple_endpoints = json.dumps({
+                "environment-uuid": "DEAD-BEEF",
+                "unit-name": "service/0",
+                "api-addresses": "10.0.3.1:17070 10.0.3.2:18080",
+                "private-address": "127.0.0.1"})
+
+        stub_config = self.Config(self.makeFile(juju_multiple_endpoints))
+        juju_info = get_juju_info(stub_config)
+        self.assertEqual(
+            {u"environment-uuid": "DEAD-BEEF",
+             u"unit-name": "service/0",
+             u"api-addresses": ["10.0.3.1:17070", "10.0.3.2:18080"],
+             u"private-address": "127.0.0.1"}, juju_info)
 
=== modified file 'landscape/message_schemas.py'
--- landscape/message_schemas.py	2013-09-25 08:43:53 +0000
+++ landscape/message_schemas.py	2013-09-27 14:18:49 +0000
@@ -103,8 +103,12 @@
 HARDWARE_INFO = Message("hardware-info", {
     "data": utf8})
 
-JUJU_INFO = Message("juju-info", {
-    "data": Dict(utf8, utf8)})
+juju_data = {"environment-uuid": utf8,
+             "api-addresses": List(utf8),
+             "unit-name": utf8}
+
+# The copy is needed because Message mutates the dictionary
+JUJU_INFO = Message("juju-info", juju_data.copy())
 
 LOAD_AVERAGE = Message("load-average", {
     "load-averages": List(Tuple(Int(), Float())),
@@ -171,9 +175,11 @@
      "hostname": utf8,
      "account_name": utf8,
      "tags": Any(utf8, Constant(None)),
-     "vm-info": String()},
+     "vm-info": String(),
+     "juju-info": KeyDict(juju_data)},
     # hostname wasn't around in old versions
-    optional=["registration_password", "hostname", "tags", "vm-info"])
+    optional=["registration_password", "hostname", "tags", "vm-info",
+              "juju-info"])
 
 
 REGISTER_PROVISIONED_MACHINE = Message(
 
=== modified file 'landscape/monitor/config.py'
--- landscape/monitor/config.py	2013-09-25 14:06:05 +0000
+++ landscape/monitor/config.py	2013-09-27 14:18:49 +0000
@@ -1,5 +1,3 @@
-import os.path
-
 from landscape.deployment import Configuration
 
 
@@ -32,8 +30,3 @@
         if self.monitor_plugins == "ALL":
             return ALL_PLUGINS
         return [x.strip() for x in self.monitor_plugins.split(",")]
-
-    @property
-    def juju_filename(self):
-        """Get the path to the Juju JSON file."""
-        return os.path.join(self.data_path, "juju-info.json")
 
=== modified file 'landscape/monitor/jujuinfo.py'
--- landscape/monitor/jujuinfo.py	2013-09-25 14:06:05 +0000
+++ landscape/monitor/jujuinfo.py	2013-09-27 14:18:49 +0000
@@ -1,8 +1,6 @@
-import json
 import logging
-import os.path
 
-from landscape.lib.fs import read_file
+from landscape.lib.juju import get_juju_info
 from landscape.monitor.plugin import MonitorPlugin
 
 
@@ -21,32 +19,16 @@
         broker.call_if_accepted("juju-info", self.send_juju_message, urgent)
 
     def send_juju_message(self, urgent=False):
-        message = {}
-        juju_data = self._create_juju_info_message()
-        if juju_data:
+        message = self._create_juju_info_message()
+        if message:
             message["type"] = "juju-info"
-            message["data"] = juju_data
             logging.info("Queuing message with updated juju info.")
             self.registry.broker.send_message(message, self._session_id,
                                               urgent=urgent)
 
     def _create_juju_info_message(self):
-        message = self._get_juju_info()
+        message = get_juju_info(self.registry.config)
         if message != self._persist.get("juju-info"):
             self._persist.set("juju-info", message)
             return message
         return None
-
-    def _get_juju_info(self):
-        juju_filename = self.registry.config.juju_filename
-        if not os.path.isfile(juju_filename):
-            return None
-        json_contents = read_file(juju_filename)
-        try:
-            juju_info = json.loads(json_contents)
-        except Exception:
-            logging.exception(
-                "Error attempting to read JSON from %s" % juju_filename)
-            return None
-        else:
-            return juju_info
 
=== modified file 'landscape/monitor/tests/test_config.py'
--- landscape/monitor/tests/test_config.py	2013-09-25 14:06:05 +0000
+++ landscape/monitor/tests/test_config.py	2013-09-27 14:18:49 +0000
@@ -1,5 +1,3 @@
-import os.path
-
 from landscape.tests.helpers import LandscapeTest
 from landscape.monitor.config import MonitorConfiguration, ALL_PLUGINS
 
@@ -32,12 +30,3 @@
         """
         self.config.load(["--flush-interval", "123"])
         self.assertEqual(self.config.flush_interval, 123)
-
-    def test_juju_filename(self):
-        """
-        Can get path to juju JSON file from config.
-        """
-        self.config.data_path = self.makeDir()
-        self.assertEqual(self.config.juju_filename, os.path.join(
-                self.config.data_path, "juju-info.json"))
-
 
=== modified file 'landscape/monitor/tests/test_jujuinfo.py'
--- landscape/monitor/tests/test_jujuinfo.py	2013-09-25 14:06:05 +0000
+++ landscape/monitor/tests/test_jujuinfo.py	2013-09-27 14:18:49 +0000
@@ -30,10 +30,9 @@
         self.plugin.exchange()
         message = self.mstore.get_pending_messages()[0]
         self.assertEqual(message["type"], "juju-info")
-        self.assertEqual(message["data"]["environment-uuid"], "DEAD-BEEF")
-        self.assertEqual(message["data"]["unit-name"], "juju-unit-name")
-        self.assertEqual(message["data"]["api-addresses"], "10.0.3.1:17070")
-        self.assertEqual(message["data"]["private-address"], "127.0.0.1")
+        self.assertEqual(message["environment-uuid"], "DEAD-BEEF")
+        self.assertEqual(message["unit-name"], "juju-unit-name")
+        self.assertEqual(message["api-addresses"], ["10.0.3.1:17070"])
 
     def test_juju_info_reported_only_once(self):
         """
@@ -57,10 +56,9 @@
         self.plugin.exchange()
         message = self.mstore.get_pending_messages()[0]
         self.assertEqual(message["type"], "juju-info")
-        self.assertEqual(message["data"]["environment-uuid"], "DEAD-BEEF")
-        self.assertEqual(message["data"]["unit-name"], "juju-unit-name")
-        self.assertEqual(message["data"]["api-addresses"], "10.0.3.1:17070")
-        self.assertEqual(message["data"]["private-address"], "127.0.0.1")
+        self.assertEqual(message["environment-uuid"], "DEAD-BEEF")
+        self.assertEqual(message["unit-name"], "juju-unit-name")
+        self.assertEqual(message["api-addresses"], ["10.0.3.1:17070"])
 
         self.makeFile(
             json.dumps({"environment-uuid": "FEED-BEEF",
@@ -71,10 +69,9 @@
         self.plugin.exchange()
         message = self.mstore.get_pending_messages()[1]
         self.assertEqual(message["type"], "juju-info")
-        self.assertEqual(message["data"]["environment-uuid"], "FEED-BEEF")
-        self.assertEqual(message["data"]["unit-name"], "changed-unit-name")
-        self.assertEqual(message["data"]["api-addresses"], "10.0.3.2:17070")
-        self.assertEqual(message["data"]["private-address"], "127.0.1.1")
+        self.assertEqual(message["environment-uuid"], "FEED-BEEF")
+        self.assertEqual(message["unit-name"], "changed-unit-name")
+        self.assertEqual(message["api-addresses"], ["10.0.3.2:17070"])
 
     def test_no_message_with_invalid_json(self):
         """No Juju message is sent if the JSON file is invalid."""
 
=== modified file 'landscape/tests/test_deployment.py'
--- landscape/tests/test_deployment.py	2013-09-25 09:24:17 +0000
+++ landscape/tests/test_deployment.py	2013-09-27 14:18:49 +0000
@@ -560,7 +560,7 @@
 
         If a specific config file is requested, use this instead of defaults.
 
-        If a cmdilne --config option is specified this should take precedence
+        If a cmdline --config option is specified this should take precedence
         over either of the former options.
         """
         default_filename1 = self.makeFile("")
@@ -599,8 +599,9 @@
         The L{Configuration.socket_path} property returns the path to the
         socket directory.
         """
-        self.assertEqual(self.config.sockets_path,
-                         "/var/lib/landscape/client/sockets")
+        self.assertEqual(
+            "/var/lib/landscape/client/sockets",
+            self.config.sockets_path)
 
     def test_annotations_path(self):
         """
@@ -611,6 +612,15 @@
             "/var/lib/landscape/client/annotations.d",
             self.config.annotations_path)
 
+    def test_juju_filename(self):
+        """
+        The L{Configuration.juju_filename} property returns the path to the
+        juju info file.
+        """
+        self.assertEqual(
+            "/var/lib/landscape/client/juju-info.json",
+            self.config.juju_filename)
+
     def test_clone(self):
         """The L{Configuration.clone} method clones a configuration."""
         self.write_config_file()