launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #19208
[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
