← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~wgrant/launchpad/webhook-trigger into lp:launchpad

 

William Grant has proposed merging lp:~wgrant/launchpad/webhook-trigger into lp:launchpad.

Commit message:
Provide WebhookSet.trigger, and include X-Launchpad-Delivery and X-Launchpad-Event-Type headers.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/webhook-trigger/+merge/267488

Provide WebhookSet.trigger, and include X-Launchpad-Delivery and X-Launchpad-Event-Type headers.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/webhook-trigger into lp:launchpad.
=== modified file 'lib/lp/services/webhooks/client.py'
--- lib/lp/services/webhooks/client.py	2015-08-03 08:27:04 +0000
+++ lib/lp/services/webhooks/client.py	2015-08-10 09:33:28 +0000
@@ -18,11 +18,13 @@
 from lp.services.webhooks.interfaces import IWebhookClient
 
 
-def create_request(user_agent, secret, payload):
+def create_request(user_agent, secret, delivery_id, event_type, payload):
     body = json.dumps(payload)
     headers = {
         'User-Agent': user_agent,
         'Content-Type': 'application/json',
+        'X-Launchpad-Event-Type': event_type,
+        'X-Launchpad-Delivery': delivery_id,
         }
     if secret is not None:
         hexdigest = hmac.new(secret, body, digestmod=hashlib.sha1).hexdigest()
@@ -33,7 +35,8 @@
 @implementer(IWebhookClient)
 class WebhookClient:
 
-    def deliver(self, url, proxy, user_agent, timeout, secret, payload):
+    def deliver(self, url, proxy, user_agent, timeout, secret, delivery_id,
+                event_type, payload):
         """See `IWebhookClient`."""
         # We never want to execute a job if there's no proxy configured, as
         # we'd then be sending near-arbitrary requests from a trusted
@@ -49,7 +52,8 @@
         session.trust_env = False
         session.headers = {}
 
-        body, headers = create_request(user_agent, secret, payload)
+        body, headers = create_request(
+            user_agent, secret, delivery_id, event_type, payload)
         preq = session.prepare_request(requests.Request(
             'POST', url, data=body, headers=headers))
 

=== modified file 'lib/lp/services/webhooks/interfaces.py'
--- lib/lp/services/webhooks/interfaces.py	2015-08-10 06:39:16 +0000
+++ lib/lp/services/webhooks/interfaces.py	2015-08-10 09:33:28 +0000
@@ -181,6 +181,9 @@
     def findByTarget(target):
         """Find all webhooks for the given target."""
 
+    def trigger(target, event_type, payload):
+        """Trigger subscribed webhooks to deliver a payload."""
+
 
 class IWebhookTarget(Interface):
 
@@ -265,6 +268,9 @@
         description=_("Timestamp of the last delivery attempt."),
         required=False, readonly=True))
 
+    event_type = exported(
+        TextLine(title=_('Event type'), required=True, readonly=True))
+
     payload = exported(Dict(
         title=_('Event payload'),
         key_type=TextLine(), required=True, readonly=True))
@@ -296,7 +302,8 @@
 
 class IWebhookClient(Interface):
 
-    def deliver(self, url, proxy, user_agent, timeout, secret, payload):
+    def deliver(self, url, proxy, user_agent, timeout, secret, delivery_id,
+                event_type, payload):
         """Deliver a payload to a webhook endpoint.
 
         Returns a dict of request and response details. The 'request' key

=== modified file 'lib/lp/services/webhooks/model.py'
--- lib/lp/services/webhooks/model.py	2015-08-10 06:39:16 +0000
+++ lib/lp/services/webhooks/model.py	2015-08-10 09:33:28 +0000
@@ -128,7 +128,7 @@
         return self.deliveries.find(WebhookJob.job_id == id).one()
 
     def ping(self):
-        return WebhookDeliveryJob.create(self, {'ping': True})
+        return WebhookDeliveryJob.create(self, 'ping', {'ping': True})
 
     def destroySelf(self):
         getUtility(IWebhookSet).delete([self])
@@ -191,6 +191,14 @@
         return IStore(Webhook).find(Webhook, target_filter).order_by(
             Webhook.id)
 
+    def trigger(self, target, event_type, payload):
+        # XXX wgrant 2015-08-10: Two INSERTs and one celery submission
+        # for each webhook, but the set should be small and we'd have to
+        # defer the triggering itself to a job to fix it.
+        for webhook in self.findByTarget(target):
+            if webhook.active and event_type in webhook.event_types:
+                WebhookDeliveryJob.create(webhook, event_type, payload)
+
 
 class WebhookTargetMixin:
 
@@ -320,9 +328,10 @@
     config = config.IWebhookDeliveryJobSource
 
     @classmethod
-    def create(cls, webhook, payload):
+    def create(cls, webhook, event_type, payload):
         webhook_job = WebhookJob(
-            webhook, cls.class_job_type, {"payload": payload})
+            webhook, cls.class_job_type,
+            {"event_type": event_type, "payload": payload})
         job = cls(webhook_job)
         job.celeryRunOnCommit()
         return job
@@ -364,6 +373,10 @@
         return iso8601.parse_date(self.json_data['date_sent'])
 
     @property
+    def event_type(self):
+        return self.json_data['event_type']
+
+    @property
     def payload(self):
         return self.json_data['payload']
 
@@ -410,7 +423,7 @@
         result = getUtility(IWebhookClient).deliver(
             self.webhook.delivery_url, config.webhooks.http_proxy,
             user_agent, 30, secret.encode('utf-8') if secret else None,
-            self.payload)
+            str(self.job_id), self.event_type, self.payload)
         # Request and response headers and body may be large, so don't
         # store them in the frequently-used JSON. We could store them in
         # the librarian if we wanted them in future.

=== modified file 'lib/lp/services/webhooks/tests/test_job.py'
--- lib/lp/services/webhooks/tests/test_job.py	2015-08-04 13:37:56 +0000
+++ lib/lp/services/webhooks/tests/test_job.py	2015-08-10 09:33:28 +0000
@@ -28,6 +28,7 @@
     LessThan,
     MatchesAll,
     MatchesDict,
+    MatchesRegex,
     MatchesStructure,
     Not,
     )
@@ -149,7 +150,8 @@
         with HTTMock(endpoint_mock):
             result = WebhookClient().deliver(
                 'http://example.com/ep', 'http://squid.example.com:3128',
-                'TestWebhookClient', 30, 'sekrit', {'foo': 'bar'})
+                'TestWebhookClient', 30, 'sekrit', '1234', 'test',
+                {'foo': 'bar'})
 
         return reqs, result
 
@@ -158,12 +160,14 @@
         return MatchesDict({
             'url': Equals('http://example.com/ep'),
             'method': Equals('POST'),
-            'headers': Equals(
-                {'Content-Type': 'application/json',
-                 'Content-Length': '14',
-                 'User-Agent': 'TestWebhookClient',
-                 'X-Hub-Signature':
-                    'sha1=de75f136c37d89f5eb24834468c1ecd602fa95dd',
+            'headers': MatchesDict(
+                {'Content-Type': Equals('application/json'),
+                 'Content-Length': Equals('14'),
+                 'User-Agent': Equals('TestWebhookClient'),
+                 'X-Launchpad-Event-Type': Equals('test'),
+                 'X-Launchpad-Delivery': MatchesRegex(r'\d+'),
+                 'X-Hub-Signature': Equals(
+                    'sha1=de75f136c37d89f5eb24834468c1ecd602fa95dd'),
                  }),
             'body': Equals('{"foo": "bar"}'),
             })
@@ -215,8 +219,10 @@
         self.raises = raises
         self.requests = []
 
-    def deliver(self, url, proxy, user_agent, timeout, secret, payload):
-        body, headers = create_request(user_agent, secret, payload)
+    def deliver(self, url, proxy, user_agent, timeout, secret, delivery_id,
+                event_type, payload):
+        body, headers = create_request(
+            user_agent, secret, delivery_id, event_type, payload)
         result = {
             'request': {
                 'url': url,
@@ -245,7 +251,7 @@
         hook = self.factory.makeWebhook(
             delivery_url=u'http://example.com/ep', secret=secret,
             active=active)
-        job = WebhookDeliveryJob.create(hook, payload={'foo': 'bar'})
+        job = WebhookDeliveryJob.create(hook, 'test', payload={'foo': 'bar'})
 
         client = MockWebhookClient(
             response_status=response_status, raises=raises)
@@ -255,11 +261,15 @@
             JobRunner([job]).runAll()
         return job, client.requests
 
-    def test_provides_interface(self):
+    def test_create(self):
         # `WebhookDeliveryJob` objects provide `IWebhookDeliveryJob`.
         hook = self.factory.makeWebhook()
-        self.assertProvides(
-            WebhookDeliveryJob.create(hook, payload={}), IWebhookDeliveryJob)
+        job = WebhookDeliveryJob.create(hook, 'test', payload={'foo': 'bar'})
+        self.assertProvides(job, IWebhookDeliveryJob)
+        self.assertThat(
+            job,
+            MatchesStructure.byEquality(
+                webhook=hook, event_type='test', payload={'foo': 'bar'}))
 
     def test_short_lease_and_timeout(self):
         # Webhook jobs have a request timeout of 30 seconds, a celery
@@ -298,7 +308,9 @@
         self.assertEqual([
             ('POST', 'http://example.com/ep',
              {'Content-Type': 'application/json',
-              'User-Agent': 'launchpad.dev-Webhooks/r%s' % revno}),
+              'User-Agent': 'launchpad.dev-Webhooks/r%s' % revno,
+              'X-Launchpad-Event-Type': 'test',
+              'X-Launchpad-Delivery': str(job.job_id)}),
             ], reqs)
         self.assertEqual([], oopses.oopses)
 
@@ -313,7 +325,9 @@
              {'Content-Type': 'application/json',
               'User-Agent': 'launchpad.dev-Webhooks/r%s' % revno,
               'X-Hub-Signature':
-                'sha1=de75f136c37d89f5eb24834468c1ecd602fa95dd'}),
+                'sha1=de75f136c37d89f5eb24834468c1ecd602fa95dd',
+              'X-Launchpad-Event-Type': 'test',
+              'X-Launchpad-Delivery': str(job.job_id)}),
             ], reqs)
         self.assertEqual([], oopses.oopses)
 
@@ -462,7 +476,7 @@
 
     def test_automatic_retries(self):
         hook = self.factory.makeWebhook()
-        job = WebhookDeliveryJob.create(hook, payload={'foo': 'bar'})
+        job = WebhookDeliveryJob.create(hook, 'test', payload={'foo': 'bar'})
         client = MockWebhookClient(response_status=404)
         self.useFixture(ZopeUtilityFixture(client, IWebhookClient))
 
@@ -496,7 +510,7 @@
 
     def test_manual_retries(self):
         hook = self.factory.makeWebhook()
-        job = WebhookDeliveryJob.create(hook, payload={'foo': 'bar'})
+        job = WebhookDeliveryJob.create(hook, 'test', payload={'foo': 'bar'})
         client = MockWebhookClient(response_status=404)
         self.useFixture(ZopeUtilityFixture(client, IWebhookClient))
 
@@ -544,7 +558,7 @@
         # retries can be resumed. This can be useful for recovering from
         # systemic errors that erroneously failed many deliveries.
         hook = self.factory.makeWebhook()
-        job = WebhookDeliveryJob.create(hook, payload={'foo': 'bar'})
+        job = WebhookDeliveryJob.create(hook, 'test', payload={'foo': 'bar'})
         client = MockWebhookClient(response_status=404)
         self.useFixture(ZopeUtilityFixture(client, IWebhookClient))
 
@@ -571,7 +585,7 @@
 
     def test_run_from_cronscript(self):
         hook = self.factory.makeWebhook(delivery_url=u'http://example.com/ep')
-        job = WebhookDeliveryJob.create(hook, payload={'foo': 'bar'})
+        job = WebhookDeliveryJob.create(hook, 'test', payload={'foo': 'bar'})
         self.assertEqual(JobStatus.WAITING, job.status)
         transaction.commit()
 
@@ -601,7 +615,8 @@
         self.useFixture(FeatureFixture(
             {'jobs.celery.enabled_classes': 'WebhookDeliveryJob'}))
         with block_on_job():
-            job = WebhookDeliveryJob.create(hook, payload={'foo': 'bar'})
+            job = WebhookDeliveryJob.create(
+                hook, 'test', payload={'foo': 'bar'})
             transaction.commit()
 
         self.assertEqual(JobStatus.WAITING, job.status)

=== modified file 'lib/lp/services/webhooks/tests/test_model.py'
--- lib/lp/services/webhooks/tests/test_model.py	2015-08-10 06:39:16 +0000
+++ lib/lp/services/webhooks/tests/test_model.py	2015-08-10 09:33:28 +0000
@@ -6,6 +6,7 @@
 from testtools.matchers import (
     Equals,
     GreaterThan,
+    HasLength,
     )
 import transaction
 from zope.component import getUtility
@@ -191,3 +192,39 @@
              getUtility(IWebhookSet).findByTarget(target)])
         self.assertEqual(1, IStore(WebhookJob).find(WebhookJob).count())
         self.assertEqual(1, hooks[2].deliveries.count())
+
+    def test_trigger(self):
+        owner = self.factory.makePerson()
+        target1 = self.factory.makeGitRepository(owner=owner)
+        target2 = self.factory.makeGitRepository(owner=owner)
+        hook1a = self.factory.makeWebhook(
+            target=target1, event_types=[])
+        hook1b = self.factory.makeWebhook(
+            target=target1, event_types=['git:push:0.1'])
+        hook2a = self.factory.makeWebhook(
+            target=target2, event_types=['git:push:0.1'])
+        hook2b = self.factory.makeWebhook(
+            target=target2, event_types=['git:push:0.1'], active=False)
+
+        # Only webhooks subscribed to the relevant target and event type
+        # are triggered.
+        getUtility(IWebhookSet).trigger(
+            target1, 'git:push:0.1', {'some': 'payload'})
+        with admin_logged_in():
+            self.assertThat(list(hook1a.deliveries), HasLength(0))
+            self.assertThat(list(hook1b.deliveries), HasLength(1))
+            self.assertThat(list(hook2a.deliveries), HasLength(0))
+            self.assertThat(list(hook2b.deliveries), HasLength(0))
+            delivery = hook1b.deliveries.one()
+            self.assertEqual(delivery.payload, {'some': 'payload'})
+
+        # Disabled webhooks aren't triggered.
+        getUtility(IWebhookSet).trigger(
+            target2, 'git:push:0.1', {'other': 'payload'})
+        with admin_logged_in():
+            self.assertThat(list(hook1a.deliveries), HasLength(0))
+            self.assertThat(list(hook1b.deliveries), HasLength(1))
+            self.assertThat(list(hook2a.deliveries), HasLength(1))
+            self.assertThat(list(hook2b.deliveries), HasLength(0))
+            delivery = hook2a.deliveries.one()
+            self.assertEqual(delivery.payload, {'other': 'payload'})

=== modified file 'lib/lp/services/webhooks/tests/test_webservice.py'
--- lib/lp/services/webhooks/tests/test_webservice.py	2015-08-10 05:50:41 +0000
+++ lib/lp/services/webhooks/tests/test_webservice.py	2015-08-10 09:33:28 +0000
@@ -216,10 +216,11 @@
             MatchesAll(
                 KeysEqual(
                     'date_created', 'date_first_sent', 'date_sent',
-                    'error_message', 'http_etag', 'payload', 'pending',
-                    'resource_type_link', 'self_link', 'successful',
-                    'web_link', 'webhook_link'),
+                    'error_message', 'event_type', 'http_etag', 'payload',
+                    'pending', 'resource_type_link', 'self_link',
+                    'successful', 'web_link', 'webhook_link'),
                 ContainsDict({
+                    'event_type': Equals('ping'),
                     'payload': Equals({'ping': True}),
                     'pending': Equals(True),
                     'successful': Is(None),

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2015-08-10 06:39:16 +0000
+++ lib/lp/testing/factory.py	2015-08-10 09:33:28 +0000
@@ -4526,13 +4526,14 @@
             LiveFSFile(livefsbuild=livefsbuild, libraryfile=libraryfile))
 
     def makeWebhook(self, target=None, delivery_url=None, secret=None,
-                    active=True):
+                    active=True, event_types=None):
         if target is None:
             target = self.makeGitRepository()
         if delivery_url is None:
             delivery_url = self.getUniqueURL().decode('utf-8')
         return getUtility(IWebhookSet).new(
-            target, self.makePerson(), delivery_url, [], active, secret)
+            target, self.makePerson(), delivery_url, event_types or [],
+            active, secret)
 
     def makeSnap(self, registrant=None, owner=None, distroseries=None,
                  name=None, branch=None, git_ref=None,


Follow ups