launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/rename-maasserver-provisioning into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/rename-maasserver-provisioning/+merge/119936

There isn't much left of maasserver.provisioning.  It's just compose_preseed and its helpers now.  So it deserves a new name.  I tried merging it into preseed.py, but the whole thing got way too big.  Maybe we'll have a preseed directory containing smaller modules at some point.

The other thing I ran into when I tried merging the two was the feared and loathed import/model-registration problem.  Weird tracebacks while trying to run tests.  Just as I was ready to give up I stumbled upon the source: maasserver.models forces an early import of the module.  That was done originally for the purpose of installing signal handlers that forward model changes to pserv and Cobbler.  But we no longer have or need those signal handlers, and thus the early import can go as well.  One more piece of technical compromise out the window.

Some tests for the signal handlers' behaviour followed as well.  These were good tests actually, thoughtful and still correct, but the purpose they served for us no longer exists.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/rename-maasserver-provisioning/+merge/119936
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/rename-maasserver-provisioning into lp:maas.

=== renamed file 'src/maasserver/provisioning.py' => 'src/maasserver/compose_preseed.py'
--- src/maasserver/provisioning.py	2012-08-16 09:50:00 +0000
+++ src/maasserver/compose_preseed.py	2012-08-16 13:48:44 +0000
@@ -1,7 +1,7 @@
 # Copyright 2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Interact with the Provisioning API."""
+"""Low-level composition code for preseeds."""
 
 from __future__ import (
     absolute_import,

=== modified file 'src/maasserver/models/__init__.py'
--- src/maasserver/models/__init__.py	2012-08-08 13:41:22 +0000
+++ src/maasserver/models/__init__.py	2012-08-16 13:48:44 +0000
@@ -101,11 +101,6 @@
                     perm)
 
 
-# 'provisioning' is imported so that it can register its signal handlers early
-# on, before it misses anything.
-from maasserver import provisioning
-ignore_unused(provisioning)
-
 from maasserver import messages
 ignore_unused(messages)
 

=== modified file 'src/maasserver/preseed.py'
--- src/maasserver/preseed.py	2012-08-14 09:24:03 +0000
+++ src/maasserver/preseed.py	2012-08-16 13:48:44 +0000
@@ -21,11 +21,11 @@
 from urllib import urlencode
 
 from django.conf import settings
+from maasserver.compose_preseed import compose_preseed
 from maasserver.enum import (
     NODE_STATUS,
     PRESEED_TYPE,
     )
-from maasserver.provisioning import compose_preseed
 from maasserver.server_address import get_maas_facing_server_host
 from maasserver.utils import absolute_reverse
 import tempita

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-08-15 03:20:44 +0000
+++ src/maasserver/tests/test_api.py	2012-08-16 13:48:44 +0000
@@ -31,7 +31,6 @@
 from django.conf import settings
 from django.contrib.auth.models import AnonymousUser
 from django.core.urlresolvers import reverse
-from django.db.models.signals import post_save
 from django.http import QueryDict
 from fixtures import Fixture
 from maasserver import (
@@ -354,27 +353,6 @@
             ["Mac address %s already in use." % mac],
             parsed_result['mac_addresses'])
 
-    def test_POST_fails_if_mac_duplicated_does_not_trigger_post_save(self):
-        # Mac Addresses should be unique, if the check fails,
-        # Node.post_save is not triggered.
-        mac = 'aa:bb:cc:dd:ee:ff'
-        factory.make_mac_address(mac)
-        architecture = factory.getRandomChoice(ARCHITECTURE_CHOICES)
-
-        def node_created(sender, instance, created, **kwargs):
-            self.fail("post_save should not have been called")
-
-        post_save.connect(node_created, sender=Node)
-        self.addCleanup(post_save.disconnect, node_created, sender=Node)
-        self.client.post(
-            self.get_uri('nodes/'),
-            {
-                'op': 'new',
-                'architecture': architecture,
-                'hostname': factory.getRandomString(),
-                'mac_addresses': [mac],
-            })
-
     def test_POST_fails_with_bad_operation(self):
         # If the operation ('op=operation_name') specified in the
         # request data is unknown, a 'Bad request' response is returned.
@@ -2207,33 +2185,6 @@
             (httplib.INTERNAL_SERVER_ERROR, error_message),
             (response.status_code, response.content))
 
-    def test_Node_post_save_error_rollbacks_transaction(self):
-        # If post_save raises an exception after a Node is added, the
-        # whole transaction is rolledback.
-        error_message = factory.getRandomString()
-
-        def raise_exception(*args, **kwargs):
-            raise RuntimeError(error_message)
-        post_save.connect(raise_exception, sender=Node)
-        self.addCleanup(post_save.disconnect, raise_exception, sender=Node)
-
-        architecture = factory.getRandomChoice(ARCHITECTURE_CHOICES)
-        hostname = factory.getRandomString()
-        response = self.client.post(self.get_uri('nodes/'), {
-            'op': 'new',
-            'hostname': hostname,
-            'architecture': architecture,
-            'after_commissioning_action':
-                NODE_AFTER_COMMISSIONING_ACTION.DEFAULT,
-            'mac_addresses': ['aa:bb:cc:dd:ee:ff'],
-        })
-
-        self.assertEqual(
-            (httplib.INTERNAL_SERVER_ERROR, error_message),
-            (response.status_code, response.content))
-        self.assertRaises(
-            Node.DoesNotExist, Node.objects.get, hostname=hostname)
-
 
 class TestAnonymousCommissioningTimeout(APIv10TestMixin, TestCase):
     """Testing of commissioning timeout API."""

=== renamed file 'src/maasserver/tests/test_provisioning.py' => 'src/maasserver/tests/test_compose_preseed.py'
--- src/maasserver/tests/test_provisioning.py	2012-08-16 09:50:00 +0000
+++ src/maasserver/tests/test_compose_preseed.py	2012-08-16 13:48:44 +0000
@@ -1,7 +1,7 @@
 # Copyright 2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Tests for `maasserver.provisioning`."""
+"""Tests for `maasserver.compsoe_preseed`."""
 
 from __future__ import (
     absolute_import,
@@ -12,8 +12,8 @@
 __metaclass__ = type
 __all__ = []
 
+from maasserver.compose_preseed import compose_preseed
 from maasserver.enum import NODE_STATUS
-from maasserver.provisioning import compose_preseed
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import TestCase
 from maasserver.utils import absolute_reverse
@@ -25,8 +25,7 @@
 import yaml
 
 
-class TestHelpers(TestCase):
-    """Tests for helpers that don't actually need any kind of pserv."""
+class TestComposePreseed(TestCase):
 
     def test_compose_preseed_for_commissioning_node_produces_yaml(self):
         node = factory.make_node(status=NODE_STATUS.COMMISSIONING)