launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05441
[Merge] lp:~allenap/txlongpoll/oops-amqp into lp:txlongpoll
Gavin Panella has proposed merging lp:~allenap/txlongpoll/oops-amqp into lp:txlongpoll with lp:~allenap/txlongpoll/bootstrap-without-net as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~allenap/txlongpoll/oops-amqp/+merge/81504
This builds on Rob's work and adds some simple tests for the plugin code.
--
https://code.launchpad.net/~allenap/txlongpoll/oops-amqp/+merge/81504
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/txlongpoll/oops-amqp into lp:txlongpoll.
=== modified file 'Makefile'
--- Makefile 2011-11-07 23:02:26 +0000
+++ Makefile 2011-11-07 23:02:26 +0000
@@ -22,8 +22,8 @@
$(BUILDOUT)
-check: bin/test
- bin/test -vv
+check: bin/testpy
+ bin/testpy -m subunit/run -- discover
dist: $(BUILDOUT_BIN)
@@ -58,7 +58,7 @@
$(BUILDOUT) install runtime
-bin/test: $(BUILDOUT_BIN) $(BUILDOUT_CFG) setup.py
+bin/testpy: $(BUILDOUT_BIN) $(BUILDOUT_CFG) setup.py
$(BUILDOUT) install test
=== added file 'NEWS'
--- NEWS 1970-01-01 00:00:00 +0000
+++ NEWS 2011-11-07 23:02:26 +0000
@@ -0,0 +1,11 @@
+txlongpoll NEWS
++++++++++++++++
+
+Changes and improvements to txlongpoll, grouped by release.
+
+NEXT
+----
+
+* OOPS reports can be reported directly over AMQP if the oops-exchange option
+ is given. The oops-prefix option has been renamed to oops-reporter to fit in
+ with the long term terminology oops has adopted. (Robert Collins)
=== modified file 'buildout.cfg'
--- buildout.cfg 2011-11-07 23:02:26 +0000
+++ buildout.cfg 2011-11-07 23:02:26 +0000
@@ -20,13 +20,12 @@
[runtime]
recipe = zc.recipe.egg:scripts
eggs = txlongpoll
-entry-points = twistd=twisted.scripts.twistd:run
-interpreter = py
+entry-points = twistd=twisted.scripts.twistd:run
[test]
-recipe = zc.recipe.testrunner
+recipe = zc.recipe.egg:scripts
eggs = txlongpoll [test]
-defaults = '--tests-pattern ^tests --exit-with-status'.split()
+interpreter = testpy
[tags]
recipe = z3c.recipe.tag:tags
=== modified file 'setup.py'
--- setup.py 2011-11-07 23:02:26 +0000
+++ setup.py 2011-11-07 23:02:26 +0000
@@ -18,6 +18,7 @@
zip_safe=False,
description='Long polling HTTP frontend for AMQP',
install_requires=[
+ 'oops_amqp',
'oops_datedir_repo',
'oops_twisted >= 0.0.3',
'setproctitle',
@@ -30,5 +31,6 @@
'rabbitfixture',
'testresources >= 0.2.4_r58',
'testtools',
+ # 'subunit', # Not easy-installable.
],
))
=== modified file 'twisted/plugins/txlongpoll.py'
--- twisted/plugins/txlongpoll.py 2011-10-06 20:15:29 +0000
+++ twisted/plugins/txlongpoll.py 2011-11-07 23:02:26 +0000
@@ -3,144 +3,9 @@
from __future__ import absolute_import
-import signal
-import sys
-
-from oops_datedir_repo import DateDirRepo
-from oops_twisted import (
- Config as oops_config,
- defer_publisher,
- OOPSObserver,
- )
-import setproctitle
-from twisted.application.internet import (
- TCPClient,
- TCPServer,
- )
-from twisted.application.service import (
- IServiceMaker,
- MultiService,
- )
-from twisted.internet import reactor
-from twisted.plugin import IPlugin
-from twisted.python import (
- log,
- usage,
- )
-from twisted.python.log import (
- addObserver,
- FileLogObserver,
- )
-from twisted.python.logfile import LogFile
-from twisted.web.server import Site
-from txlongpoll.client import AMQFactory
-from txlongpoll.frontend import (
- FrontEndAjax,
- QueueManager,
- )
-from zope.interface import implements
-
-
-def getRotatableLogFileObserver(filename):
- """Setup a L{LogFile} for the given application."""
- if filename != '-':
- logfile = LogFile.fromFullPath(
- filename, rotateLength=None, defaultMode=0644)
-
- def signal_handler(*args):
- reactor.callFromThread(logfile.reopen)
-
- signal.signal(signal.SIGUSR1, signal_handler)
- else:
- logfile = sys.stdout
-
- return FileLogObserver(logfile)
-
-
-def setUpOopsHandler(options, logfile):
- """Add OOPS handling based on the passed command line options."""
- config = oops_config()
-
- # Add the oops publisher that writes files in the configured place
- # if the command line option was set.
- if options["oops-dir"]:
- repo = DateDirRepo(options["oops-dir"], options["oops-prefix"])
- config.publishers.append(defer_publisher(repo.publish))
-
- observer = OOPSObserver(config, logfile.emit)
- addObserver(observer.emit)
-
-
-class Options(usage.Options):
- optParameters = [
- ["logfile", "l", "txlongpoll.log", "Logfile name."],
- ["brokerport", "p", 5672, "Broker port"],
- ["brokerhost", "h", '127.0.0.1', "Broker host"],
- ["brokeruser", "u", None, "Broker user"],
- ["brokerpassword", "a", None, "Broker password"],
- ["brokervhost", "v", '/', "Broker vhost"],
- ["frontendport", "f", None, "Frontend port"],
- ["prefix", "x", None, "Queue prefix"],
- ["oops-dir", "r", None, "Where to write OOPS reports"],
- ["oops-prefix", "o", "LONGPOLL", "String prefix for OOPS IDs"],
- ]
-
- def postOptions(self):
- for man_arg in ('frontendport', 'brokeruser', 'brokerpassword'):
- if not self[man_arg]:
- raise usage.UsageError("--%s must be specified." % man_arg)
- for int_arg in ('brokerport', 'frontendport'):
- try:
- self[int_arg] = int(self[int_arg])
- except (TypeError, ValueError):
- raise usage.UsageError("--%s must be an integer." % int_arg)
-
-
-class AMQServiceMaker(object):
- """Create an asynchronous frontend server for AMQP."""
-
- implements(IServiceMaker, IPlugin)
-
- options = Options
-
- def __init__(self, name, description):
- self.tapname = name
- self.description = description
-
- def makeService(self, options):
- """Construct a TCPServer and TCPClient."""
- setproctitle.setproctitle(
- "txlongpoll: accepting connections on %s" %
- options["frontendport"])
-
- logfile = getRotatableLogFileObserver(options["logfile"])
- setUpOopsHandler(options, logfile)
-
- broker_port = options["brokerport"]
- broker_host = options["brokerhost"]
- broker_user = options["brokeruser"]
- broker_password = options["brokerpassword"]
- broker_vhost = options["brokervhost"]
- frontend_port = options["frontendport"]
- prefix = options["prefix"]
-
- manager = QueueManager(prefix)
- factory = AMQFactory(
- broker_user, broker_password, broker_vhost, manager.connected,
- manager.disconnected,
- lambda (connector, reason): log.err(reason, "Connection failed"))
- resource = FrontEndAjax(manager)
-
- client_service = TCPClient(broker_host, broker_port, factory)
- server_service = TCPServer(frontend_port, Site(resource))
- services = MultiService()
- services.addService(client_service)
- services.addService(server_service)
-
- return services
-
-
-# Now construct objects which *provide* the relevant interfaces. The name of
+from txlongpoll.plugin import AMQServiceMaker
+
+# Construct objects which *provide* the relevant interfaces. The name of
# these variables is irrelevant, as long as there are *some* names bound to
# providers of IPlugin and IServiceMaker.
=== added file 'txlongpoll/plugin.py'
--- txlongpoll/plugin.py 1970-01-01 00:00:00 +0000
+++ txlongpoll/plugin.py 2011-11-07 23:02:26 +0000
@@ -0,0 +1,172 @@
+# Copyright 2005-2011 Canonical Ltd. This software is licensed under
+# the GNU Affero General Public License version 3 (see the file LICENSE).
+
+__all__ = [
+ "AMQServiceMaker",
+ ]
+
+from functools import partial
+import signal
+import sys
+
+from amqplib import client_0_8 as amqp
+import oops
+from oops_amqp import Publisher
+from oops_datedir_repo import DateDirRepo
+from oops_twisted import (
+ Config as oops_config,
+ defer_publisher,
+ OOPSObserver,
+ )
+import setproctitle
+from twisted.application.internet import (
+ TCPClient,
+ TCPServer,
+ )
+from twisted.application.service import (
+ IServiceMaker,
+ MultiService,
+ )
+from twisted.internet import reactor
+from twisted.plugin import IPlugin
+from twisted.python import (
+ log,
+ usage,
+ )
+from twisted.python.log import (
+ addObserver,
+ FileLogObserver,
+ )
+from twisted.python.logfile import LogFile
+from twisted.web.server import Site
+from txlongpoll.client import AMQFactory
+from txlongpoll.frontend import (
+ FrontEndAjax,
+ QueueManager,
+ )
+from zope.interface import implements
+
+
+def getRotatableLogFileObserver(filename):
+ """Setup a L{LogFile} for the given application."""
+ if filename != '-':
+ logfile = LogFile.fromFullPath(
+ filename, rotateLength=None, defaultMode=0644)
+ def signal_handler(sig, frame):
+ reactor.callFromThread(logfile.reopen)
+ signal.signal(signal.SIGUSR1, signal_handler)
+ else:
+ logfile = sys.stdout
+ return FileLogObserver(logfile)
+
+
+def setUpOOPSHandler(options, logfile):
+ """Add OOPS handling based on the passed command line options."""
+ config = oops_config()
+
+ # Add the oops publisher that writes files in the configured place
+ # if the command line option was set.
+
+ if options["oops-exchange"]:
+ oops_exchange = options["oops-exchange"]
+ oops_key = options["oops-routingkey"] or ""
+ host = options["brokerhost"]
+ if options["brokerport"]:
+ host = "%s:%s" % (host, options["brokerport"])
+ rabbit_connect = partial(
+ amqp.Connection, host=host,
+ userid=options["brokeruser"],
+ password=options["brokerpassword"],
+ virtual_host=options["brokervhost"])
+ amqp_publisher = Publisher(
+ rabbit_connect, oops_exchange, oops_key)
+ config.publishers.append(defer_publisher(amqp_publisher))
+
+ if options["oops-dir"]:
+ repo = DateDirRepo(options["oops-dir"])
+ config.publishers.append(
+ defer_publisher(oops.publish_new_only(repo.publish)))
+
+ if options["oops-reporter"]:
+ config.template['reporter'] = options["oops-reporter"]
+
+ observer = OOPSObserver(config, logfile.emit)
+ addObserver(observer.emit)
+ return observer
+
+
+class Options(usage.Options):
+
+ optParameters = [
+ ["logfile", "l", "txlongpoll.log", "Logfile name."],
+ ["brokerport", "p", 5672, "Broker port"],
+ ["brokerhost", "h", '127.0.0.1', "Broker host"],
+ ["brokeruser", "u", None, "Broker user"],
+ ["brokerpassword", "a", None, "Broker password"],
+ ["brokervhost", "v", '/', "Broker vhost"],
+ ["frontendport", "f", None, "Frontend port"],
+ ["prefix", "x", None, "Queue prefix"],
+ ["oops-dir", "r", None, "Where to write OOPS reports"],
+ ["oops-reporter", "o", "LONGPOLL", "String identifying this service."],
+ ["oops-exchange", None, None, "AMQP Exchange to send OOPS reports to."],
+ ["oops-routingkey", None, None, "Routing key for AMQP OOPSes."],
+ ]
+
+ def postOptions(self):
+ for man_arg in ('frontendport', 'brokeruser', 'brokerpassword'):
+ if not self[man_arg]:
+ raise usage.UsageError("--%s must be specified." % man_arg)
+ for int_arg in ('brokerport', 'frontendport'):
+ try:
+ self[int_arg] = int(self[int_arg])
+ except (TypeError, ValueError):
+ raise usage.UsageError("--%s must be an integer." % int_arg)
+ if not self["oops-reporter"] and (
+ self["oops-exchange"] or self["oops-dir"]):
+ raise usage.UsageError(
+ "A reporter must be supplied to identify reports "
+ "from this service from other OOPS reports.")
+
+
+class AMQServiceMaker(object):
+ """Create an asynchronous frontend server for AMQP."""
+
+ implements(IServiceMaker, IPlugin)
+
+ options = Options
+
+ def __init__(self, name, description):
+ self.tapname = name
+ self.description = description
+
+ def makeService(self, options):
+ """Construct a TCPServer and TCPClient."""
+ setproctitle.setproctitle(
+ "txlongpoll: accepting connections on %s" %
+ options["frontendport"])
+
+ logfile = getRotatableLogFileObserver(options["logfile"])
+ setUpOOPSHandler(options, logfile)
+
+ broker_port = options["brokerport"]
+ broker_host = options["brokerhost"]
+ broker_user = options["brokeruser"]
+ broker_password = options["brokerpassword"]
+ broker_vhost = options["brokervhost"]
+ frontend_port = options["frontendport"]
+ prefix = options["prefix"]
+
+ manager = QueueManager(prefix)
+ factory = AMQFactory(
+ broker_user, broker_password, broker_vhost, manager.connected,
+ manager.disconnected,
+ lambda (connector, reason): log.err(reason, "Connection failed"))
+ resource = FrontEndAjax(manager)
+
+ client_service = TCPClient(broker_host, broker_port, factory)
+ server_service = TCPServer(frontend_port, Site(resource))
+ services = MultiService()
+ services.addService(client_service)
+ services.addService(server_service)
+
+ return services
=== added file 'txlongpoll/tests/test_plugin.py'
--- txlongpoll/tests/test_plugin.py 1970-01-01 00:00:00 +0000
+++ txlongpoll/tests/test_plugin.py 2011-11-07 23:02:26 +0000
@@ -0,0 +1,243 @@
+# Copyright 2005-2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from cStringIO import StringIO
+from functools import partial
+from unittest import defaultTestLoader
+
+from fixtures import TempDir
+from oops_twisted import OOPSObserver
+from subunit import IsolatedTestCase
+from testtools import TestCase
+from testtools.content import (
+ Content,
+ UTF8_TEXT,
+ )
+from testtools.matchers import (
+ MatchesException,
+ Raises,
+ )
+from twisted.application.service import MultiService
+from twisted.python.log import (
+ FileLogObserver,
+ theLogPublisher,
+ )
+from twisted.python.usage import UsageError
+from txlongpoll.plugin import (
+ AMQServiceMaker,
+ Options,
+ setUpOOPSHandler,
+ )
+
+
+def options_diff(a, b):
+ diff = []
+ for name in sorted(set().union(a, b)):
+ if name in a and name in b:
+ if a[name] != b[name]:
+ diff.append((name, a[name], b[name]))
+ elif name in a:
+ diff.append((name, a[name], None))
+ elif name in b:
+ diff.append((name, None, b[name]))
+ return diff
+
+
+class TestOptions(TestCase):
+ """Tests for `txlongpoll.plugin.Options`."""
+
+ def test_defaults(self):
+ options = Options()
+ expected = {
+ "brokerhost": "127.0.0.1",
+ "brokerpassword": None,
+ "brokerport": 5672,
+ "brokeruser": None,
+ "brokervhost": "/",
+ "frontendport": None,
+ "logfile": "txlongpoll.log",
+ "oops-dir": None,
+ "oops-exchange": None,
+ "oops-reporter": "LONGPOLL",
+ "oops-routingkey": None,
+ "prefix": None,
+ }
+ self.assertEqual(expected, options.defaults)
+
+ def test_parse_minimal_options(self):
+ # Some options are mandatory.
+ options = Options()
+
+ def check_exception(message, *arguments):
+ self.assertThat(
+ partial(options.parseOptions, arguments),
+ Raises(MatchesException(UsageError, message)))
+
+ check_exception(
+ "--frontendport must be specified")
+ check_exception(
+ "--brokeruser must be specified",
+ "--frontendport", "1234")
+ check_exception(
+ "--brokerpassword must be specified",
+ "--brokeruser", "Bob",
+ "--frontendport", "1234")
+
+ # The minimal set of options that must be provided.
+ arguments = [
+ "--brokerpassword", "Hoskins",
+ "--brokeruser", "Bob",
+ "--frontendport", "1234",
+ ]
+ options.parseOptions(arguments) # No error.
+
+ def test_parse_int_options(self):
+ # Some options are converted to ints.
+ options = Options()
+ arguments = [
+ "--brokerpassword", "Hoskins",
+ "--brokerport", "4321",
+ "--brokeruser", "Bob",
+ "--frontendport", "1234",
+ ]
+ options.parseOptions(arguments)
+ expected_diff = [
+ ("brokerpassword", None, "Hoskins"),
+ ("brokerport", 5672, 4321),
+ ("brokeruser", None, "Bob"),
+ ("frontendport", None, 1234),
+ ]
+ observed_diff = options_diff(options.defaults, options)
+ self.assertEqual(expected_diff, observed_diff)
+
+ def test_parse_broken_int_options(self):
+ # An error is raised if the integer options do not contain integers.
+ options = Options()
+ arguments = [
+ "--brokerpassword", "Hoskins",
+ "--brokerport", "Jr.",
+ "--brokeruser", "Bob",
+ ]
+ self.assertRaises(
+ UsageError, options.parseOptions, arguments)
+
+ def test_oops_exchange_without_reporter(self):
+ # It is an error to omit the OOPS reporter if exchange is specified.
+ options = Options()
+ arguments = [
+ "--brokerpassword", "Hoskins",
+ "--brokeruser", "Bob",
+ "--frontendport", "1234",
+ "--oops-exchange", "something",
+ "--oops-reporter", "",
+ ]
+ expected = MatchesException(
+ UsageError, "A reporter must be supplied")
+ self.assertThat(
+ partial(options.parseOptions, arguments),
+ Raises(expected))
+
+ def test_oops_dir_without_reporter(self):
+ # It is an error to omit the OOPS reporter if directory is specified.
+ options = Options()
+ arguments = [
+ "--brokerpassword", "Hoskins",
+ "--brokeruser", "Bob",
+ "--frontendport", "1234",
+ "--oops-dir", "/some/where",
+ "--oops-reporter", "",
+ ]
+ expected = MatchesException(
+ UsageError, "A reporter must be supplied")
+ self.assertThat(
+ partial(options.parseOptions, arguments),
+ Raises(expected))
+
+
+class TestSetUpOOPSHandler(TestCase):
+ """Tests for `txlongpoll.plugin.setUpOOPSHandler`."""
+
+ def setUp(self):
+ super(TestSetUpOOPSHandler, self).setUp()
+ self.observers = theLogPublisher.observers[:]
+ self.logfile = StringIO()
+ self.addDetail("log", Content(UTF8_TEXT, self.logfile.getvalue))
+ self.log = FileLogObserver(self.logfile)
+
+ def tearDown(self):
+ super(TestSetUpOOPSHandler, self).tearDown()
+ theLogPublisher.observers[:] = self.observers
+
+ def makeObserver(self, settings):
+ options = Options()
+ options["brokerpassword"] = "Hoskins"
+ options["brokeruser"] = "Bob"
+ options["frontendport"] = 1234
+ options.update(settings)
+ observer = setUpOOPSHandler(options, self.log)
+ return options, observer
+
+ def test_minimal(self):
+ options, observer = self.makeObserver({})
+ self.assertIsInstance(observer, OOPSObserver)
+ self.assertEqual([], observer.config.publishers)
+ self.assertEqual(
+ {"reporter": options.defaults["oops-reporter"]},
+ observer.config.template)
+
+ def test_with_all_the_things(self):
+ settings = {
+ "oops-exchange": "Frank",
+ "oops-reporter": "Sidebottom",
+ "oops-dir": self.useFixture(TempDir()).path,
+ }
+ options, observer = self.makeObserver(settings)
+ self.assertIsInstance(observer, OOPSObserver)
+ self.assertEqual(2, len(observer.config.publishers))
+ self.assertEqual(
+ {"reporter": "Sidebottom"},
+ observer.config.template)
+
+ def test_with_some_of_the_things(self):
+ settings = {
+ "oops-exchange": "Frank",
+ "oops-reporter": "Sidebottom",
+ }
+ options, observer = self.makeObserver(settings)
+ self.assertIsInstance(observer, OOPSObserver)
+ self.assertEqual(1, len(observer.config.publishers))
+ self.assertEqual(
+ {"reporter": "Sidebottom"},
+ observer.config.template)
+
+
+class TestAMQServiceMaker(IsolatedTestCase, TestCase):
+ """Tests for `txlongpoll.plugin.AMQServiceMaker`."""
+
+ def test_init(self):
+ service_maker = AMQServiceMaker("Harry", "Hill")
+ self.assertEqual("Harry", service_maker.tapname)
+ self.assertEqual("Hill", service_maker.description)
+
+ def makeOptions(self, settings):
+ options = Options()
+ options["brokerpassword"] = "Hoskins"
+ options["brokeruser"] = "Bob"
+ options["frontendport"] = 1234
+ options.update(settings)
+ return options
+
+ def test_makeService(self):
+ options = self.makeOptions({})
+ service_maker = AMQServiceMaker("Harry", "Hill")
+ service = service_maker.makeService(options)
+ self.assertIsInstance(service, MultiService)
+ self.assertEqual(2, len(service.services))
+ client_service, server_service = service.services
+ self.assertEqual(options["brokerhost"], client_service.args[0])
+ self.assertEqual(options["brokerport"], client_service.args[1])
+ self.assertEqual(options["frontendport"], server_service.args[0])
+
+
+def test_suite():
+ return defaultTestLoader.loadTestsFromName(__name__)
