launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/maas/pserv-with-optional-rabbit into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~allenap/maas/pserv-with-optional-rabbit/+merge/91108

This makes the RabbitMQ configuration arguments optional, and does not set up the AMQP client unless arguments are passed in. It also fixes some lint, and gets the buildout-related rules in Makefile to update the modified timestamp of their targets (because buildout doesn't if it detects it doesn't need to make a change).

-- 
https://code.launchpad.net/~allenap/maas/pserv-with-optional-rabbit/+merge/91108
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/pserv-with-optional-rabbit into lp:maas.

=== modified file 'Makefile'
--- Makefile	2012-01-30 15:03:37 +0000
+++ Makefile	2012-02-01 15:18:20 +0000
@@ -1,6 +1,10 @@
 PYTHON = python2.7
 
-build: bin/buildout bin/maas bin/test.maas bin/test.pserv bin/py bin/ipy
+build: \
+    bin/buildout \
+    bin/maas bin/test.maas \
+    bin/twistd.pserv bin/test.pserv \
+    bin/py bin/ipy
 
 all: build doc
 
@@ -10,18 +14,27 @@
 
 bin/maas bin/test.maas: bin/buildout buildout.cfg setup.py
 	bin/buildout install maas
+	@touch --no-create $@
+
+bin/twistd.pserv: bin/buildout buildout.cfg setup.py
+	bin/buildout install pserv
+	@touch --no-create $@
 
 bin/test.pserv: bin/buildout buildout.cfg setup.py
 	bin/buildout install pserv-test
+	@touch --no-create $@
 
 bin/flake8: bin/buildout buildout.cfg setup.py
 	bin/buildout install flake8
+	@touch --no-create $@
 
 bin/sphinx: bin/buildout buildout.cfg setup.py
 	bin/buildout install sphinx
+	@touch --no-create $@
 
 bin/py bin/ipy: bin/buildout buildout.cfg setup.py
 	bin/buildout install repl
+	@touch --no-create $@
 
 dev-db:
 	utilities/maasdb start ./db/ disposable

=== modified file 'buildout.cfg'
--- buildout.cfg	2012-01-26 17:23:04 +0000
+++ buildout.cfg	2012-02-01 15:18:20 +0000
@@ -60,10 +60,10 @@
   twisted
   txamqp
 entry-points =
-  pserv=twisted.scripts.twistd:run
+  twistd.pserv=twisted.scripts.twistd:run
 extra-paths = ${common:extra-paths}
 scripts =
-  pserv
+  twistd.pserv
 
 [pserv-test]
 recipe = zc.recipe.egg

=== modified file 'src/maas/urls.py'
--- src/maas/urls.py	2012-01-24 12:44:48 +0000
+++ src/maas/urls.py	2012-02-01 15:18:20 +0000
@@ -13,12 +13,13 @@
 
 from django.conf import settings
 from django.conf.urls.defaults import (
+    include,
     patterns,
-    include,
     url,
     )
 from django.contrib.staticfiles.urls import staticfiles_urlpatterns
 
+
 urlpatterns = patterns('',
     url(r'^', include('maasserver.urls')),
 )

=== modified file 'src/provisioningserver/amqpclient.py'
--- src/provisioningserver/amqpclient.py	2012-01-26 12:50:56 +0000
+++ src/provisioningserver/amqpclient.py	2012-02-01 15:18:20 +0000
@@ -12,6 +12,11 @@
     unicode_literals,
     )
 
+__metaclass__ = type
+__all__ = [
+    "AMQFactory",
+    ]
+
 import os.path
 
 from twisted.internet.defer import maybeDeferred
@@ -21,11 +26,6 @@
 from txamqp.queue import Closed
 from txamqp.spec import load as load_spec
 
-__metaclass__ = type
-__all__ = [
-    "AMQFactory",
-    ]
-
 
 class AMQClientWithCallback(AMQClient):
     """

=== modified file 'src/provisioningserver/plugin.py'
--- src/provisioningserver/plugin.py	2012-01-26 23:08:42 +0000
+++ src/provisioningserver/plugin.py	2012-02-01 15:18:20 +0000
@@ -8,10 +8,10 @@
     unicode_literals,
     )
 
-import setproctitle
 import signal
 import sys
 
+from amqpclient import AMQFactory
 import oops
 from oops_datedir_repo import DateDirRepo
 from oops_twisted import (
@@ -19,6 +19,7 @@
     defer_publisher,
     OOPSObserver,
     )
+import setproctitle
 from twisted.application.internet import TCPClient
 from twisted.application.service import (
     IServiceMaker,
@@ -30,14 +31,13 @@
     log,
     usage,
     )
-from twisted.python.logfile import LogFile
 from twisted.python.log import (
     addObserver,
     FileLogObserver,
     )
+from twisted.python.logfile import LogFile
 from zope.interface import implements
 
-from amqpclient import AMQFactory
 
 __metaclass__ = type
 __all__ = []
@@ -92,9 +92,6 @@
         ]
 
     def postOptions(self):
-        for arg in ('brokeruser', 'brokerpassword'):
-            if not self[arg]:
-                raise usage.UsageError("--%s must be specified." % arg)
         for int_arg in ('brokerport',):
             try:
                 self[int_arg] = int(self[int_arg])
@@ -133,28 +130,26 @@
         logfile = getRotatableLogFileObserver(options["logfile"])
         setUpOOPSHandler(options, logfile)
 
+        services = MultiService()
+
         broker_port = options["brokerport"]
         broker_host = options["brokerhost"]
         broker_user = options["brokeruser"]
         broker_password = options["brokerpassword"]
         broker_vhost = options["brokervhost"]
 
-        # TODO: define callbacks for Rabbit connectivity.
-        # e.g. construct a manager object.
-        client_factory = AMQFactory(
-            broker_user, broker_password, broker_vhost,
-            CONNECTED_CALLBACK, DISCONNECTED_CALLBACK,
-            lambda (connector, reason): log.err(reason, "Connection failed"))
-
-        # TODO: Create services here, e.g.
-        # service1 = thing
-        # service2 = thing2
-        # services = MultiService()
-        # services.addService(service1)
-        # services.addService(service2)
-        # return services
-
-        client_service = TCPClient(broker_host, broker_port, client_factory)
-        services = MultiService()
-        services.addService(client_service)
+        # Connecting to RabbitMQ is optional; it is not yet a required
+        # component of a running MaaS installation.
+        if broker_user is not None and broker_password is not None:
+            cb_connected = lambda ignored: None  # TODO
+            cb_disconnected = lambda ignored: None  # TODO
+            cb_failed = lambda (connector, reason): (
+                log.err(reason, "Connection failed"))
+            client_factory = AMQFactory(
+                broker_user, broker_password, broker_vhost,
+                cb_connected, cb_disconnected, cb_failed)
+            client_service = TCPClient(
+                broker_host, broker_port, client_factory)
+            services.addService(client_service)
+
         return services

=== modified file 'src/provisioningserver/testing/amqpclient.py'
--- src/provisioningserver/testing/amqpclient.py	2012-01-26 13:13:13 +0000
+++ src/provisioningserver/testing/amqpclient.py	2012-02-01 15:18:20 +0000
@@ -1,6 +1,17 @@
 # Copyright 2005-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+"""Tests for `provisioningserver.amqpclient`."""
+
+from __future__ import (
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+from provisioningserver.amqpclient import AMQFactory
 from rabbitfixture.server import RabbitServer
 from testresources import (
     FixtureResource,
@@ -17,7 +28,6 @@
     inlineCallbacks,
     )
 from txamqp.client import Closed
-from provisioningserver.amqpclient import AMQFactory
 
 
 class QueueWrapper(object):

=== modified file 'src/provisioningserver/tests/test_amqaclient.py'
--- src/provisioningserver/tests/test_amqaclient.py	2012-01-26 23:08:42 +0000
+++ src/provisioningserver/tests/test_amqaclient.py	2012-02-01 15:18:20 +0000
@@ -11,14 +11,14 @@
 __metaclass__ = type
 __all__ = []
 
+from provisioningserver.amqpclient import AMQFactory
+from provisioningserver.testing.amqpclient import AMQTest
 from testtools import TestCase
 from testtools.deferredruntest import flush_logged_errors
 from twisted.internet.defer import Deferred
 from txamqp.protocol import AMQChannel
 from txamqp.queue import Closed
 from txamqp.spec import Spec
-from provisioningserver.amqpclient import AMQFactory
-from provisioningserver.testing.amqpclient import AMQTest
 
 
 class AMQFactoryTest(TestCase):

=== modified file 'src/provisioningserver/tests/test_cobblersession.py'
--- src/provisioningserver/tests/test_cobblersession.py	2012-01-31 16:09:25 +0000
+++ src/provisioningserver/tests/test_cobblersession.py	2012-02-01 15:18:20 +0000
@@ -388,4 +388,8 @@
     def test_name_method_appends_s_for_plural(self):
         self.assertEqual(
             'x_systems_y',
+<<<<<<< TREE
             cobblerclient.CobblerSystem._name_method('x_%s_y', plural=True))
+=======
+            cobblerclient.CobblerSystem.name_method('x_%s_y', plural=True))
+>>>>>>> MERGE-SOURCE

=== modified file 'src/provisioningserver/tests/test_plugin.py'
--- src/provisioningserver/tests/test_plugin.py	2012-01-26 23:08:42 +0000
+++ src/provisioningserver/tests/test_plugin.py	2012-02-01 15:18:20 +0000
@@ -14,9 +14,15 @@
 from cStringIO import StringIO
 from functools import partial
 import os
+from unittest import skip
 
 from fixtures import TempDir
 from oops_twisted import OOPSObserver
+from provisioningserver.plugin import (
+    Options,
+    ProvisioningServiceMaker,
+    setUpOOPSHandler,
+    )
 from testtools import TestCase
 from testtools.content import (
     Content,
@@ -32,11 +38,6 @@
     theLogPublisher,
     )
 from twisted.python.usage import UsageError
-from provisioningserver.plugin import (
-    ProvisioningServiceMaker,
-    Options,
-    setUpOOPSHandler,
-    )
 
 
 class TestOptions(TestCase):
@@ -62,12 +63,18 @@
             partial(options.parseOptions, arguments),
             Raises(MatchesException(UsageError, message)))
 
+    @skip(
+        "RabbitMQ is not yet a required component "
+        "of a running MaaS installation.")
     def test_option_brokeruser_required(self):
         options = Options()
         self.check_exception(
             options,
             "--brokeruser must be specified")
 
+    @skip(
+        "RabbitMQ is not yet a required component "
+        "of a running MaaS installation.")
     def test_option_brokerpassword_required(self):
         options = Options()
         self.check_exception(