Merge lp:~tribaal/landscape-client/rename-metadata-annotations into lp:~landscape/landscape-client/trunk

[Chris Glass]

This branch renames the meta-data.d/ directory to annotations.d/

This branch renames the meta-data.d/ directory to annotations.d/

One small non-related change too, where the log for the User credential were referring to "metadata" while it is in fact not meta data.

I would like to get an initial review on this, but I may have to retarget the branch depending on the release strategy we want to adopt (should we release the whole trunk since last release or just backport changes).

[lp:~tribaal/landscape-client/rename-metadata-annotations]

726. By Chris Glass on 2013-09-17

Fix typo in docstring

[Alberto Donato (ack)]

Great! +1

#1:
- meta_data_dir = self.monitor.config.meta_data_path
+ meta_data_dir = self.monitor.config.annotations_path
         os.mkdir(meta_data_dir)
         create_file(os.path.join(meta_data_dir, "juju-env-uuid"), "uuid1")
         create_file(os.path.join(meta_data_dir, "juju-unit-name"), "unit/0")

I'd s/test_meta_data/test_annotations/ for those tests, and s/meta_data_dir/annotations_dir/.
Also it'd be nice to use generic key/values, not juju info specifically, as we're going to push them in a different message.

review: Approve

[Alberto Donato (ack)]

Just as a note, the landscape-client charm will be broken when this lands, as it relies on config.meta_data_path.
My charm branch (lp:~ack/charms/precise/landscape-client/separate-juju-info) in review fixes that.

[lp:~tribaal/landscape-client/rename-metadata-annotations]

727. By Chris Glass on 2013-09-17

Addresses comment #1 - using generic annotations instead of juju-specific ones
to avoid confusion.
Renamed variables and test methods.

[Free Ekanayaka (free.ekanayaka)]

A few things to fix, re the releasing strategy we agreed on backporting, which should be a straight patch as we didn't touch much this code (if at all).

[1]

We need to disable the fetching of cloud meta data too (which would currently end up in the annotations). It's a very small change and should go in this branch.

[2]

The ComputerInfo plugin is still using "meta-data" as message type name (line 92).

[3]

The message type name is still "meta-data" in landscape/message_schemas.py as well.

review: Needs Fixing

[Chris Glass (tribaal)]

I have in progress bugs and branches for those exact points - would you rather have me smash them all together in this branch?

Works for me either way.

[lp:~tribaal/landscape-client/rename-metadata-annotations]

728. By Chris Glass on 2013-09-18

Merging second development branch into this one.
The merge branch changes the message key from "meta-data" to "annotations".

Remains to be done: One branch to deactivate the EC2 reporting code (in a separate branch so that we can reactivate it easily in the future).

729. By Chris Glass on 2013-09-18

Reanamed internal variable from meta_data to annotations.

730. By Chris Glass on 2013-09-18

Lint fixes

[Free Ekanayaka (free.ekanayaka)]

Thanks, looks good to me, it seems the only point left is [1] and will go in the other branch. +1!

review: Approve

[lp:~tribaal/landscape-client/rename-metadata-annotations]

731. By Chris Glass on 2013-09-18

Deactivate EC2 reporting for the time being.

732. By Chris Glass on 2013-09-18

Added bug number to "deactivated" comments.

[Chris Glass (tribaal)]

I changed my mind and added the removal in this branch since it's very cheap.

Please add a quick comment if it's still +1

[Free Ekanayaka (free.ekanayaka)]

+1!

=== modified file 'landscape/configuration.py'
--- landscape/configuration.py	2013-09-16 08:09:10 +0000
+++ landscape/configuration.py	2013-09-18 08:41:57 +0000
@@ -588,9 +588,10 @@
     """Create the client directories tree."""
     bootstrap_list = [
         BootstrapDirectory("$data_path", "landscape", "root", 0755),
-        BootstrapDirectory("$meta_data_path", "landscape", "landscape", 0755)]
+        BootstrapDirectory("$annotations_path", "landscape", "landscape",
+                           0755)]
     BootstrapList(bootstrap_list).bootstrap(
-        data_path=config.data_path, meta_data_path=config.meta_data_path)
+        data_path=config.data_path, annotations_path=config.annotations_path)
 
 
 def store_public_key_data(config, certificate_data):
 
=== modified file 'landscape/deployment.py'
--- landscape/deployment.py	2013-09-16 08:09:10 +0000
+++ landscape/deployment.py	2013-09-18 08:41:57 +0000
@@ -399,12 +399,12 @@
         return os.path.join(self.data_path, "sockets")
 
     @property
-    def meta_data_path(self):
+    def annotations_path(self):
         """
-        Return the path to the directory where additional meta-data files can
+        Return the path to the directory where additional annotation files can
         be stored.
         """
-        return os.path.join(self.data_path, "meta-data.d")
+        return os.path.join(self.data_path, "annotations.d")
 
 
 def get_versioned_persist(service):
 
=== modified file 'landscape/message_schemas.py'
--- landscape/message_schemas.py	2013-08-21 16:33:46 +0000
+++ landscape/message_schemas.py	2013-09-18 08:41:57 +0000
@@ -69,12 +69,10 @@
     {"hostname": utf8,
      "total-memory": Int(),
      "total-swap": Int(),
-     "meta-data": Dict(utf8, utf8),
-     "extra-meta-data": Dict(utf8, utf8)},
+     "annotations": Dict(utf8, utf8)},
     # Not sure why these are all optional, but it's explicitly tested
     # in the server
-    optional=["hostname", "total-memory", "total-swap", "meta-data",
-              "extra-meta-data"])
+    optional=["hostname", "total-memory", "total-swap", "annotations"])
 
 DISTRIBUTION_INFO = Message(
     "distribution-info",
 
=== modified file 'landscape/monitor/computerinfo.py'
--- landscape/monitor/computerinfo.py	2013-09-07 22:06:20 +0000
+++ landscape/monitor/computerinfo.py	2013-09-18 08:41:57 +0000
@@ -36,7 +36,7 @@
 
     def register(self, registry):
         super(ComputerInfo, self).register(registry)
-        self._meta_data_path = registry.config.meta_data_path
+        self._annotations_path = registry.config.annotations_path
         self.call_on_accepted("computer-info",
                               self.send_computer_message, True)
         self.call_on_accepted("distribution-info",
@@ -75,21 +75,23 @@
         self._add_if_new(message, "total-memory",
                          total_memory)
         self._add_if_new(message, "total-swap", total_swap)
-        meta_data = {}
-        if os.path.exists(self._meta_data_path):
-            for key in os.listdir(self._meta_data_path):
-                meta_data[key] = read_file(
-                    os.path.join(self._meta_data_path, key))
+        annotations = {}
+        if os.path.exists(self._annotations_path):
+            for key in os.listdir(self._annotations_path):
+                annotations[key] = read_file(
+                    os.path.join(self._annotations_path, key))
 
         if (self._cloud_meta_data is None and
             self._cloud_retries < METADATA_RETRY_MAX):
             self._cloud_meta_data = yield self._fetch_ec2_meta_data()
 
-        if self._cloud_meta_data:
-            meta_data = dict(
-                meta_data.items() + self._cloud_meta_data.items())
-        if meta_data:
-            self._add_if_new(message, "meta-data", meta_data)
+        # XXX: Deactivated EC2 reporting for the time being, until #1226605 is
+        #      implemented.
+        if False:  # if self._cloud_meta_data:
+            annotations = dict(
+                annotations.items() + self._cloud_meta_data.items())
+        if annotations:
+            self._add_if_new(message, "annotations", annotations)
         returnValue(message)
 
     def _add_if_new(self, message, key, value):
 
=== modified file 'landscape/monitor/tests/test_computerinfo.py'
--- landscape/monitor/tests/test_computerinfo.py	2013-09-06 19:48:16 +0000
+++ landscape/monitor/tests/test_computerinfo.py	2013-09-18 08:41:57 +0000
@@ -310,9 +310,15 @@
         computer_info = {"type": "computer-info", "hostname": "ooga.local",
                          "timestamp": 0, "total-memory": 1510,
                          "total-swap": 1584,
-                         "meta-data": {u"ami-id": u"ami-00002",
-                                       u"instance-id": u"i00001",
-                                       u"instance-type": u"hs1.8xlarge"}}
+                         "annotations": {u"ami-id": u"ami-00002",
+                                         u"instance-id": u"i00001",
+                                         u"instance-type": u"hs1.8xlarge"}}
+        # XXX: The tested code is deactivated, so this will not produce
+        #      annotations for the time being. It should be plugged in again
+        #      once #1226605 is implemented.
+        computer_info = {"type": "computer-info", "hostname": "ooga.local",
+                         "timestamp": 0, "total-memory": 1510,
+                         "total-swap": 1584}
         dist_info = {"type": "distribution-info",
                      "code-name": "dapper", "description": "Ubuntu 6.06.1 LTS",
                      "distributor-id": "Ubuntu", "release": "6.06"}
@@ -358,9 +364,9 @@
         self.mstore.set_accepted_types(["distribution-info", "computer-info"])
         self.assertMessages(list(self.mstore.get_pending_messages()), [])
 
-    def test_meta_data(self):
+    def test_annotations(self):
         """
-        L{ComputerInfo} sends extra meta data from the meta-data.d directory
+        L{ComputerInfo} sends extra meta data from the annotations.d directory
         if it's present.
 
         Each file name is used as a key in the meta-data dict and the file's
@@ -369,10 +375,10 @@
         This allows, for example, the landscape-client charm to send
         information about the juju environment to the landscape server.
         """
-        meta_data_dir = self.monitor.config.meta_data_path
-        os.mkdir(meta_data_dir)
-        create_file(os.path.join(meta_data_dir, "juju-env-uuid"), "uuid1")
-        create_file(os.path.join(meta_data_dir, "juju-unit-name"), "unit/0")
+        annotations_dir = self.monitor.config.annotations_path
+        os.mkdir(annotations_dir)
+        create_file(os.path.join(annotations_dir, "annotation1"), "value1")
+        create_file(os.path.join(annotations_dir, "annotation2"), "value2")
         self.mstore.set_accepted_types(["computer-info"])
 
         plugin = ComputerInfo()
@@ -381,16 +387,17 @@
         plugin.exchange()
         messages = self.mstore.get_pending_messages()
         self.assertEqual(1, len(messages))
-        meta_data = messages[0]["meta-data"]
+        meta_data = messages[0]["annotations"]
         self.assertEqual(2, len(meta_data))
-        self.assertEqual("uuid1", meta_data["juju-env-uuid"])
-        self.assertEqual("unit/0", meta_data["juju-unit-name"])
+        self.assertEqual("value1", meta_data["annotation1"])
+        self.assertEqual("value2", meta_data["annotation2"])
 
-    def test_meta_data_cloud(self):
+    def deactivated_test_annotations_cloud(self):
         """
         L{ComputerInfo} includes the meta-data key when cloud information
         is available.
         """
+        # XXX The tested code is deactivated until #1226605 is implemented.
         self.mstore.set_accepted_types(["computer-info"])
 
         plugin = ComputerInfo()
@@ -399,11 +406,12 @@
         plugin.exchange()
         messages = self.mstore.get_pending_messages()
         self.assertEqual(1, len(messages))
-        self.assertIn("meta-data", messages[0])
-        self.assertEqual("i00001", messages[0]["meta-data"]["instance-id"])
+        self.assertIn("annotations", messages[0])
+        self.assertEqual("i00001", messages[0]["annotations"]["instance-id"])
 
-    def test_with_cloud_info(self):
+    def deactivated_test_with_cloud_info(self):
         """Fetch cloud information"""
+        # XXX: The tested code is deactivated until #1226605 is implemented.
         self.config.cloud = True
         self.mstore.set_accepted_types(["computer-info"])
 
@@ -412,11 +420,11 @@
         plugin.exchange()
         messages = self.mstore.get_pending_messages()
         self.assertEqual(1, len(messages))
-        self.assertIn("meta-data", messages[0])
+        self.assertIn("annotations", messages[0])
 
         self.assertEqual({"instance-id": u"i00001", "ami-id": u"ami-00002",
                           "instance-type": u"hs1.8xlarge"},
-                         messages[0]["meta-data"])
+                         messages[0]["annotations"])
 
     def test_no_fetch_ec2_meta_data_when_cloud_retries_is_max(self):
         """
@@ -431,7 +439,7 @@
         plugin.exchange()
         messages = self.mstore.get_pending_messages()
         self.assertEqual(1, len(messages))
-        self.assertNotIn("meta-data", messages[0])
+        self.assertNotIn("annotations", messages[0])
 
     @inlineCallbacks
     def test_fetch_ec2_meta_data(self):
 
=== modified file 'landscape/tests/test_configuration.py'
--- landscape/tests/test_configuration.py	2013-09-16 07:45:45 +0000
+++ landscape/tests/test_configuration.py	2013-09-18 08:41:57 +0000
@@ -639,21 +639,21 @@
 
     def test_bootstrap_tree(self):
         """
-        The L{bootstrap_tree} function creates the client dir and /meta-data.d
-        under it with the correct permissions.
+        The L{bootstrap_tree} function creates the client dir and
+        /annotations.d under it with the correct permissions.
         """
         client_path = self.makeDir()
-        meta_data_path = os.path.join(client_path, "meta-data.d")
+        annotations_path = os.path.join(client_path, "annotations.d")
 
         mock_chmod = self.mocker.replace("os.chmod")
         mock_chmod(client_path, 0755)
-        mock_chmod(meta_data_path, 0755)
+        mock_chmod(annotations_path, 0755)
         self.mocker.replay()
 
         config = self.get_config([], data_path=client_path)
         bootstrap_tree(config)
         self.assertTrue(os.path.isdir(client_path))
-        self.assertTrue(os.path.isdir(meta_data_path))
+        self.assertTrue(os.path.isdir(annotations_path))
 
 
 class ConfigurationFunctionsTest(LandscapeConfigurationTest):
 
=== modified file 'landscape/tests/test_deployment.py'
--- landscape/tests/test_deployment.py	2013-09-16 08:10:04 +0000
+++ landscape/tests/test_deployment.py	2013-09-18 08:41:57 +0000
@@ -600,14 +600,14 @@
         self.assertEqual(self.config.sockets_path,
                          "/var/lib/landscape/client/sockets")
 
-    def test_meta_data_path(self):
+    def test_annotations_path(self):
         """
-        The L{Configuration.meta_data_path} property returns the path to the
-        socket directory.
+        The L{Configuration.annotations_path} property returns the path to the
+        annotations directory.
         """
         self.assertEqual(
-            "/var/lib/landscape/client/meta-data.d",
-            self.config.meta_data_path)
+            "/var/lib/landscape/client/annotations.d",
+            self.config.annotations_path)
 
     def test_clone(self):
         """The L{Configuration.clone} method clones a configuration."""
 
=== modified file 'landscape/user/management.py'
--- landscape/user/management.py	2008-06-17 09:49:54 +0000
+++ landscape/user/management.py	2013-09-18 08:41:57 +0000
@@ -41,7 +41,7 @@
         self._set_password(username, password)
         if require_password_reset:
             result, new_output = self.call_popen(["passwd", username, "-e"])
-            if result !=0:
+            if result != 0:
                 raise UserManagementError("Error resetting password for user "
                                           "%s.\n%s" % (username, new_output))
             else:
@@ -75,7 +75,7 @@
                          primary_group_name=None):
         """Update details for the account matching C{uid}."""
         uid = self._provider.get_uid(username)
-        logging.info("Updating metadata for user %s (UID %d).", username, uid)
+        logging.info("Updating data for user %s (UID %d).", username, uid)
         if password:
             self._set_password(username, password)
 
@@ -166,7 +166,8 @@
         gid = self._provider.get_gid(groupname)
         logging.info("Adding user %s (UID %d) to group %s (GID %d).",
                      username, uid, groupname, gid)
-        result, output = self.call_popen(["gpasswd", "-a", username, groupname])
+        result, output = self.call_popen(["gpasswd", "-a", username,
+                                          groupname])
         if result != 0:
             raise UserManagementError("Error adding user %s (UID %d) to "
                                       "group %s (GID %d).\n%s" %
@@ -182,7 +183,8 @@
         gid = self._provider.get_gid(groupname)
         logging.info("Removing user %s (UID %d) from group %s (GID %d).",
                      username, uid, groupname, gid)
-        result, output = self.call_popen(["gpasswd", "-d", username, groupname])
+        result, output = self.call_popen(["gpasswd", "-d", username,
+                                          groupname])
         if result != 0:
             raise UserManagementError("Error removing user %s (UID %d) "
                                       "from group %s (GID (%d).\n%s"