launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/maas/cobblerclient-modify into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~allenap/maas/cobblerclient-modify/+merge/92675
-- 
https://code.launchpad.net/~allenap/maas/cobblerclient-modify/+merge/92675
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/cobblerclient-modify into lp:maas.

=== modified file 'src/provisioningserver/cobblerclient.py'
--- src/provisioningserver/cobblerclient.py	2012-02-08 18:05:44 +0000
+++ src/provisioningserver/cobblerclient.py	2012-02-12 16:22:19 +0000
@@ -30,6 +30,7 @@
     'DEFAULT_TIMEOUT',
     ]
 
+from functools import partial
 import xmlrpclib
 
 from twisted.internet import reactor as default_reactor
@@ -280,6 +281,9 @@
         if "required_attributes" in attributes:
             attributes["required_attributes"] = frozenset(
                 attributes["required_attributes"])
+        if "modification_attributes" in attributes:
+            attributes["modification_attributes"] = frozenset(
+                attributes["modification_attributes"])
         return super(CobblerObjectType, mtype).__new__(
             mtype, name, bases, attributes)
 
@@ -311,6 +315,11 @@
     # What attributes does Cobbler require for this type of object?
     required_attributes = []
 
+    # What attributes do we expect to support for modifications to this
+    # object? Cobbler's API accepts some pseudo-attributes, only valid for
+    # making modifications.
+    modification_attributes = []
+
     def __init__(self, session, name):
         """Reference an object in Cobbler.
 
@@ -341,30 +350,36 @@
         return name_template % type_name
 
     @classmethod
-    def _normalize_attribute(cls, attribute_name):
+    def _normalize_attribute(cls, attribute_name, attributes=None):
         """Normalize an attribute name.
 
         Cobbler mixes dashes and underscores in attribute names.  MaaS may
         pass attributes as keyword arguments internally, where dashes are not
         an option.  Hide the distinction by looking up the proper name in
-        `known_attributes`.
+        `known_attributes` by default, but `attributes` can be passed to
+        override that.
 
         :param attribute_name: An attribute name, possibly using underscores
             where Cobbler expects dashes.
+        :param attributes: None, or a set of attributes against which to
+            match.
         :return: A Cobbler-style attribute name, using either dashes or
             underscores as used by Cobbler.
         """
-        if attribute_name in cls.known_attributes:
+        if attributes is None:
+            attributes = cls.known_attributes
+
+        if attribute_name in attributes:
             # Already spelled the way Cobbler likes it.
             return attribute_name
 
         attribute_name = attribute_name.replace('_', '-')
-        if attribute_name in cls.known_attributes:
+        if attribute_name in attributes:
             # Cobbler wants this one with dashes.
             return attribute_name
 
         attribute_name = attribute_name.replace('-', '_')
-        assert attribute_name in cls.known_attributes, (
+        assert attribute_name in attributes, (
             "Unknown attribute for %s: %s."
             % (cls.object_type, attribute_name))
         return attribute_name
@@ -467,6 +482,28 @@
         returnValue(cls(session, name))
 
     @inlineCallbacks
+    def modify(self, delta):
+        """Modify an object in Cobbler.
+
+        :param name: Identifying name for the existing object.
+        :param attributes: Dict mapping attribute names to values.
+        """
+        normalize = partial(
+            self._normalize_attribute,
+            attributes=self.modification_attributes)
+        args = {
+            normalize(key): value
+            for key, value in delta.iteritems()
+            }
+        success = yield self.session.call(
+            'xapi_object_edit', self.object_type, self.name, 'edit', args,
+            self.session.token_placeholder)
+        if not success:
+            raise RuntimeError(
+                "Cobbler refused to modify %s '%s'.  Attributes: %s"
+                % (self.object_type, self.name, args))
+
+    @inlineCallbacks
     def delete(self, recurse=True):
         """Delete this object.  Its name must be known.
 
@@ -567,6 +604,10 @@
         'kernel',
         'name',
         ]
+    modification_attributes = [
+        'initrd',
+        'kernel',
+        ]
 
 
 class CobblerRepo(CobblerObject):

=== modified file 'src/provisioningserver/testing/fakecobbler.py'
--- src/provisioningserver/testing/fakecobbler.py	2012-02-10 16:49:35 +0000
+++ src/provisioningserver/testing/fakecobbler.py	2012-02-12 16:22:19 +0000
@@ -327,6 +327,11 @@
             obj_dict.update(attrs)
             obj_dict['name'] = name
             return self._api_save_object(token, object_type, handle)
+        elif operation == 'edit':
+            handle = self._api_get_handle(token, object_type, name)
+            for key, value in attrs.iteritems():
+                self._api_modify_object(token, object_type, handle, key, value)
+            return self._api_save_object(token, object_type, handle)
         else:
             raise NotImplemented(
                 "xapi_object_edit(%s, ..., %s, ...)"

=== modified file 'src/provisioningserver/tests/test_cobblersession.py'
--- src/provisioningserver/tests/test_cobblersession.py	2012-02-06 15:14:00 +0000
+++ src/provisioningserver/tests/test_cobblersession.py	2012-02-12 16:22:19 +0000
@@ -16,7 +16,10 @@
 
 import fixtures
 from provisioningserver import cobblerclient
-from provisioningserver.testing.fakecobbler import fake_token
+from provisioningserver.testing.fakecobbler import (
+    fake_token,
+    make_fake_cobbler_session,
+    )
 from testtools.content import text_content
 from testtools.deferredruntest import (
     assert_fails_with,
@@ -399,6 +402,29 @@
                 session, 'incomplete_system', {})
 
     @inlineCallbacks
+    def test_modify(self):
+        session = make_fake_cobbler_session()
+        distro = yield cobblerclient.CobblerDistro.new(
+            session, name="fred", attributes={
+                "initrd": "an_initrd", "kernel": "a_kernel"})
+        distro_before = yield distro.get_values()
+        yield distro.modify({"kernel": "sanders"})
+        distro_after = yield distro.get_values()
+        self.assertNotEqual(distro_before, distro_after)
+        self.assertEqual("sanders", distro_after["kernel"])
+
+    @inlineCallbacks
+    def test_modify_only_permits_certain_attributes(self):
+        session = make_fake_cobbler_session()
+        distro = yield cobblerclient.CobblerDistro.new(
+            session, name="fred", attributes={
+                "initrd": "an_initrd", "kernel": "a_kernel"})
+        expected = ExpectedException(
+            AssertionError, "Unknown attribute for distro: machine")
+        with expected:
+            yield distro.modify({"machine": "head"})
+
+    @inlineCallbacks
     def test_get_values_returns_only_known_attributes(self):
         session = make_recording_session()
         # Create a new CobblerDistro. The True return value means the faked
@@ -467,3 +493,12 @@
         self.assertIsInstance(
             cobblerclient.CobblerDistro.required_attributes,
             frozenset)
+
+    def test_modification_attributes(self):
+        # modification_attributes, a class attribute, is always a frozenset.
+        self.assertIsInstance(
+            cobblerclient.CobblerObject.modification_attributes,
+            frozenset)
+        self.assertIsInstance(
+            cobblerclient.CobblerDistro.modification_attributes,
+            frozenset)

=== modified file 'src/provisioningserver/tests/test_fakecobbler.py'
--- src/provisioningserver/tests/test_fakecobbler.py	2012-02-09 17:24:16 +0000
+++ src/provisioningserver/tests/test_fakecobbler.py	2012-02-12 16:22:19 +0000
@@ -207,6 +207,15 @@
             AssertionError,
             self.cobbler_class._normalize_attribute, 'some-unknown-attribute')
 
+    def test_normalize_attribute_alternative_attributes(self):
+        # _normalize_attribute() can be passed a different set of attributes
+        # against which to normalize.
+        allowed_attributes = set(["some-unknown-attribute"])
+        self.assertEqual(
+            'some-unknown-attribute',
+            self.cobbler_class._normalize_attribute(
+                'some-unknown-attribute', allowed_attributes))
+
     @inlineCallbacks
     def test_create_object(self):
         session = yield fake_cobbler_session()