Merge lp:~free.ekanayaka/charms/trusty/landscape-client/drop-unit-name-from-juju-info into lp:~landscape/charms/trusty/landscape-client/trunk

[Free Ekanayaka]

This branch drops the unit-name field from the content of the juju info file, and changes the code to generate a single json file as opposed to one per principal unit, since we now only need the machine->computer association.

[Alberto Donato (ack)]

LGTM, +1

One question inline.

#1:
I think we could drop "private-address" too. It's optional in the message and we don't use it on the server.

review: Approve

[lp:~free.ekanayaka/charms/trusty/landscape-client/drop-unit-name-from-juju-info]

47. By Free Ekanayaka on 2014-09-25

Drop private-address from juju-info

[Free Ekanayaka (free.ekanayaka)]

#1

Fixed.

[Chris Glass (tribaal)]

+1! Ship it :)

review: Approve

=== modified file 'hooks/common.py'
--- hooks/common.py	2014-09-02 13:55:31 +0000
+++ hooks/common.py	2014-09-25 07:00:36 +0000
@@ -101,7 +101,7 @@
         is_configured_enough = all([
             self.config.get("account_name"),
             self.config.get("computer_title"),
-            get_juju_info_filenames(self.juju_directory)])
+            os.path.exists(self.juju_filename)])
         if not try_to_register:
             # We don't need to further check the configuration if we are
             # deleting it.
@@ -115,11 +115,11 @@
             return 0
 
     @property
-    def juju_directory(self):
+    def juju_filename(self):
         try:
-            return self.config.juju_directory
+            return self.config.juju_filename
         except AttributeError:  # older landscape-client versions
-            return os.path.join(self.config.data_path, "juju-info.d")
+            return os.path.join(self.config.data_path, "juju-info.json")
 
     def clear_registration(self):
         """
@@ -200,7 +200,3 @@
     """Dump content in JSON format to the specified file."""
     with open(filename, "w") as dumpfile:
         json.dump(content, dumpfile)
-
-
-def get_juju_info_filenames(juju_directory):
-    return glob("%s/*.json" % juju_directory)

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2014-08-28 12:13:31 +0000
+++ hooks/hooks.py	2014-09-25 07:00:36 +0000
@@ -11,8 +11,7 @@
 from subprocess import CalledProcessError
 
 from common import (
-    write_json_file, JujuBroker, LandscapeBroker, chown,
-    get_juju_info_filenames)
+    write_json_file, JujuBroker, LandscapeBroker, chown)
 from ceph import (
     get_ceph_client_path, write_ceph_client_keyring, write_ceph_client_config)
 
@@ -70,31 +69,14 @@
     juju_broker.log("In container-relation-joined for %s"
                     "" % landscape_broker.local_unit)
     landscape_broker.config.reload()
-    relation_conf = juju_broker.get_relation_config()
-    remote_unit_name = juju_broker.environment.get("JUJU_REMOTE_UNIT")
     # We use the remote unit for the unit name, since we want to associate this
     # client with the unit it's managing, not its own unit.
     juju_info = {
         "environment-uuid": juju_broker.environment.get("JUJU_ENV_UUID"),
-        "unit-name": remote_unit_name,
         "machine-id": juju_broker.get_machine_id(),
-        "api-addresses": juju_broker.environment.get("JUJU_API_ADDRESSES"),
-        "private-address": relation_conf.get("private-address")}
-
-    # Let's not use any "/" in a filename.
-    unit_name = remote_unit_name.replace("/", "-")
-    juju_file = "%s.json" % unit_name
-
-    juju_info_dir = landscape_broker.juju_directory
-
-    try:
-        os.mkdir(juju_info_dir)
-    except OSError:
-        pass  # The file already exists.
-
-    juju_info_file = os.path.join(juju_info_dir, juju_file)
-
-    write_json_file(juju_info_file, juju_info)
+        "api-addresses": juju_broker.environment.get("JUJU_API_ADDRESSES")}
+
+    write_json_file(landscape_broker.juju_filename, juju_info)
     exit_code = landscape_broker.update_client_config(
         {"computer-title": socket.gethostname()})
 
@@ -112,9 +94,8 @@
     landscape_broker.clear_registration()
 
     # Delete all of the juju-info files.
-    files_to_delete = get_juju_info_filenames(landscape_broker.juju_directory)
-    for to_delete in files_to_delete:
-        os.remove(to_delete)
+    if os.path.exists(landscape_broker.juju_filename):
+        os.remove(landscape_broker.juju_filename)
     landscape_broker.update_client_config(
         {"computer-title": ""}, try_to_register=False)
 
@@ -180,16 +161,18 @@
     juju_key_map = {"juju-env-uuid": "environment-uuid",
                     "juju-unit-name": "unit-name",
                     "juju-api-addresses": "api-addresses",
-                    "juju-private-address": "private-address"}
-    juju_info = dict.fromkeys(juju_key_map.values())
+                    "juju-private-address": None}
+    juju_info = dict.fromkeys(key for key in juju_key_map.values() if key)
     files_to_delete = []
     for filename, info_key in juju_key_map.iteritems():
         file_path = os.path.join(meta_data_dir, filename)
         if not os.path.exists(file_path):
             continue
+        files_to_delete.append(file_path)
+        if info_key is None:
+            continue
         with open(file_path, "r") as juju_info_part_file:
             juju_info[info_key] = juju_info_part_file.read().strip()
-        files_to_delete.append(file_path)
     missing_values = [value is None for value in juju_info.itervalues()]
     # If they're all missing it probably means we've already converted.
     if all(missing_values):

=== modified file 'hooks/test_hooks.py'
--- hooks/test_hooks.py	2014-08-28 12:13:31 +0000
+++ hooks/test_hooks.py	2014-09-25 07:00:36 +0000
@@ -5,7 +5,6 @@
 from tempfile import mkdtemp
 from unittest import TestCase
 from uuid import uuid4
-from glob import glob
 
 from landscape.deployment import Configuration
 from landscape.lib.fs import read_file
@@ -67,12 +66,8 @@
         self.landscape_broker.config.write()
         self.juju_broker = FakeJujuBroker()
 
-    def touch_example_juju_info_file(self):
-        if not os.path.exists(self.landscape_broker.juju_directory):
-            os.mkdir(self.landscape_broker.juju_directory)
-        filename = os.path.join(self.landscape_broker.juju_directory,
-                                "whatever.json")
-        write_json_file(filename=filename, content={})
+    def touch_juju_info_file(self):
+        write_json_file(self.landscape_broker.juju_filename, content={})
 
     def mkdtemp(self):
         directory = mkdtemp(prefix=self.__class__.__name__)
@@ -115,8 +110,7 @@
         self.assertEqual(
             {"environment-uuid": "0afcd28d-6263-43fb-8131-afa696a984f8",
              "unit-name": "postgresql/0",
-             "api-addresses": "10.0.3.1:17070",
-             "private-address": "10.0.3.205"}, juju_json_info)
+             "api-addresses": "10.0.3.1:17070"}, juju_json_info)
 
     def test_old_juju_info_deleted_after_successful_migration(self):
         """
@@ -175,23 +169,16 @@
         """
         self.juju_broker.environment.update({
             "JUJU_ENV_UUID": "uuid1",
-            "JUJU_REMOTE_UNIT": "unit/0",
             "JUJU_API_ADDRESSES": "10.0.0.1:1234"})
-        self.juju_broker.commands["relation-get"] = {
-            "private-address": "10.0.0.99"}
         hooks.container_relation_joined(juju_broker=self.juju_broker,
                                         landscape_broker=self.landscape_broker)
 
-        juju_info_path = os.path.join(
-            self.landscape_broker.juju_directory, "unit-0.json")
-
+        juju_info_path = self.landscape_broker.juju_filename
         juju_info = json.loads(read_file(juju_info_path))
         self.assertEqual(
             {"environment-uuid": "uuid1",
              "machine-id": "1",
-             "unit-name": "unit/0",
-             "api-addresses": "10.0.0.1:1234",
-             "private-address": "10.0.0.99"},
+             "api-addresses": "10.0.0.1:1234"},
             juju_info)
 
     def test_registers_after_juju_info_is_written(self):
@@ -203,9 +190,7 @@
         registration_attempts = []
 
         def record_registration_attempt():
-            juju_directory = self.landscape_broker.juju_directory
-            juju_file_list = glob("%s/*.json" % juju_directory)
-            for _ in juju_file_list:
+            if os.path.exists(self.landscape_broker.juju_filename):
                 registration_attempts.append(True)
 
         self.landscape_broker.try_to_register = record_registration_attempt
@@ -254,16 +239,9 @@
         since it contains information about the remove unit (which is
         now gone).
         """
-        juju_info_path = os.path.join(
-            self.landscape_broker.juju_directory, "unit-0.json")
-
-        if not os.path.exists(self.landscape_broker.juju_directory):
-            os.mkdir(self.landscape_broker.juju_directory)
-
+        juju_info_path = self.landscape_broker.juju_filename
         juju_info = {"environment-uuid": "uuid1",
-                     "unit-name": "unit/0",
-                     "api-addresses": "10.0.0.1:1234",
-                     "private-address": "10.0.0.99"}
+                     "api-addresses": "10.0.0.1:1234"}
         write_json_file(juju_info_path, juju_info)
         hooks.container_relation_departed(
             landscape_broker=self.landscape_broker)
@@ -411,7 +389,7 @@
         super(ConfigChangedTest, self).setUp()
         self.landscape_broker.config.account_name = "account1"
         self.landscape_broker.config.computer_title = "title"
-        self.touch_example_juju_info_file()
+        self.touch_juju_info_file()
         self.addCleanup(setattr, hooks, "CERT_FILE", hooks.CERT_FILE)
         tmp_dir = self.mkdtemp()
         hooks.CERT_FILE = "%s/cert" % tmp_dir
@@ -440,7 +418,7 @@
         super(RegistrationRelationJoinedTest, self).setUp()
         self.landscape_broker.config.account_name = "account1"
         self.landscape_broker.config.computer_title = "title"
-        self.touch_example_juju_info_file()
+        self.touch_juju_info_file()
         self.addCleanup(setattr, hooks, "CERT_FILE", hooks.CERT_FILE)
         tmp_dir = self.mkdtemp()
         hooks.CERT_FILE = "%s/cert" % tmp_dir