launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #19156
[Merge] lp:~wgrant/launchpad/webhook-api-and-active into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/webhook-api-and-active into lp:launchpad.
Commit message:
Webhook enhancements: secret is settable, error_message is exposed, and the active flag is respected.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/webhook-api-and-active/+merge/266835
Webhook enhancements: secret is settable, error_message is exposed, and the active flag is respected.
Also relax a couple of celery retry tests slightly, as buildbot can be astoundingly slow.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/webhook-api-and-active into lp:launchpad.
=== modified file 'lib/lp/services/job/tests/test_celery.py'
--- lib/lp/services/job/tests/test_celery.py 2015-07-24 09:17:39 +0000
+++ lib/lp/services/job/tests/test_celery.py 2015-08-04 06:04:54 +0000
@@ -155,8 +155,6 @@
self.assertEqual(JobStatus.WAITING, job_forever.status)
self.assertThat(
job_future.date_started, GreaterThan(job_past.date_started))
- self.assertThat(
- job_future.date_started, GreaterThan(job_whenever.date_started))
def test_jobs_with_retry_exceptions_are_queued_again(self):
# A job that raises a retry error is automatically queued
@@ -200,7 +198,7 @@
LessThan(dates_started[0] + timedelta(seconds=8))),
MatchesAll(
GreaterThan(dates_started[1] + timedelta(seconds=8)),
- LessThan(dates_started[1] + timedelta(seconds=12))),
+ LessThan(dates_started[1] + timedelta(seconds=13))),
]))
self.assertEqual(3, job.attempt_count)
self.assertEqual(JobStatus.COMPLETED, job.status)
=== modified file 'lib/lp/services/webhooks/interfaces.py'
--- lib/lp/services/webhooks/interfaces.py 2015-08-03 08:27:04 +0000
+++ lib/lp/services/webhooks/interfaces.py 2015-08-04 06:04:54 +0000
@@ -31,12 +31,14 @@
export_write_operation,
exported,
operation_for_version,
+ operation_parameters,
REQUEST_USER,
)
from lazr.restful.fields import (
CollectionField,
Reference,
)
+from lazr.restful.interface import copy_field
from zope.interface import (
Attribute,
Interface,
@@ -111,8 +113,13 @@
title=_("URL"), required=True, readonly=False))
active = exported(Bool(
title=_("Active"), required=True, readonly=False))
+
+ # Do not export this.
secret = TextLine(
- title=_("Unique name"), required=False, readonly=True)
+ title=_("Secret"), required=False,
+ description=_(
+ "An optional string used to sign delivery bodies with HMAC-SHA1 "
+ "in the X-Hub-Signature header."))
deliveries = exported(doNotSnapshot(CollectionField(
title=_("Recent deliveries for this webhook."),
@@ -132,6 +139,12 @@
def destroySelf():
"""Delete this webhook."""
+ @export_write_operation()
+ @operation_parameters(secret=copy_field(secret))
+ @operation_for_version('devel')
+ def setSecret(secret):
+ """Set the secret used to sign deliveries."""
+
class IWebhookSource(Interface):
@@ -159,9 +172,10 @@
@call_with(registrant=REQUEST_USER)
@export_factory_operation(
- IWebhook, ['delivery_url', 'active', 'event_types'])
+ IWebhook, ['delivery_url', 'active', 'event_types', 'secret'])
@operation_for_version("devel")
- def newWebhook(registrant, delivery_url, event_types, active=True):
+ def newWebhook(registrant, delivery_url, event_types, active=True,
+ secret=None):
"""Create a new webhook."""
@@ -210,6 +224,13 @@
"no attempts have been made yet."),
required=False, readonly=True))
+ error_message = exported(TextLine(
+ title=_("Error message"),
+ description=_(
+ "Details of the error encountered by the most recent delivery "
+ "attempt."),
+ required=False, readonly=True))
+
date_created = exported(Datetime(
title=_("Date created"), required=True, readonly=True))
@@ -228,8 +249,13 @@
key_type=TextLine(), required=True, readonly=True))
@export_write_operation()
+ @operation_parameters(reset=Bool(
+ title=_("Reset automatic retries"),
+ description=_(
+ "Restart the 24 hour automatic retry window as well as trying "
+ "again now.")))
@operation_for_version("devel")
- def retry():
+ def retry(reset=False):
"""Attempt to deliver the event again.
Launchpad will automatically retry regularly for 24 hours, but
=== modified file 'lib/lp/services/webhooks/model.py'
--- lib/lp/services/webhooks/model.py 2015-08-03 10:49:46 +0000
+++ lib/lp/services/webhooks/model.py 2015-08-04 06:04:54 +0000
@@ -31,6 +31,7 @@
)
from storm.references import Reference
from storm.store import Store
+import transaction
from zope.component import getUtility
from zope.interface import (
implementer,
@@ -144,6 +145,10 @@
updated_data['event_types'] = event_types
self.json_data = updated_data
+ def setSecret(self, secret):
+ """See `IWebhook`."""
+ self.secret = secret
+
@implementer(IWebhookSource)
class WebhookSource:
@@ -195,11 +200,12 @@
getUtility(IWebhookSource).findByTarget(self),
pre_iter_hook=preload_registrants)
- def newWebhook(self, registrant, delivery_url, event_types, active=True):
+ def newWebhook(self, registrant, delivery_url, event_types, active=True,
+ secret=None):
if not getFeatureFlag('webhooks.new.enabled'):
raise WebhookFeatureDisabled()
return getUtility(IWebhookSource).new(
- self, registrant, delivery_url, event_types, active, None)
+ self, registrant, delivery_url, event_types, active, secret)
class WebhookJobType(DBEnumeratedType):
@@ -326,12 +332,14 @@
def successful(self):
if 'result' not in self.json_data:
return None
- return self.failure_detail is None
+ return self.error_message is None
@property
- def failure_detail(self):
+ def error_message(self):
if 'result' not in self.json_data:
return None
+ if self.json_data['result'].get('webhook_deactivated'):
+ return 'Webhook deactivated'
connection_error = self.json_data['result'].get('connection_error')
if connection_error is not None:
return 'Connection error: %s' % connection_error
@@ -360,10 +368,14 @@
def _time_since_first_attempt(self):
return datetime.now(utc) - (self.date_first_sent or self.date_created)
- def retry(self):
+ def retry(self, reset=False):
"""See `IWebhookDeliveryJob`."""
# Unset any retry delay and reset attempt_count to prevent
# queue() from delaying it again.
+ if reset:
+ updated_data = self.json_data
+ del updated_data['date_first_sent']
+ self.json_data = updated_data
self.scheduled_start = None
self.attempt_count = 0
self.queue()
@@ -382,6 +394,13 @@
return timedelta(hours=1)
def run(self):
+ if not self.webhook.active:
+ updated_data = self.json_data
+ updated_data['result'] = {'webhook_deactivated': True}
+ self.json_data = updated_data
+ # Job.fail will abort the transaction.
+ transaction.commit()
+ raise WebhookDeliveryFailure(self.error_message)
user_agent = '%s-Webhooks/r%s' % (
config.vhost.mainsite.hostname, lp.app.versioninfo.revno)
secret = self.webhook.secret
@@ -402,9 +421,10 @@
if 'date_first_sent' not in updated_data:
updated_data['date_first_sent'] = updated_data['date_sent']
self.json_data = updated_data
+ transaction.commit()
if not self.successful:
if self.retry_automatically:
raise WebhookDeliveryRetry()
else:
- raise WebhookDeliveryFailure(self.failure_detail)
+ raise WebhookDeliveryFailure(self.error_message)
=== renamed file 'lib/lp/services/webhooks/tests/test_webhookjob.py' => 'lib/lp/services/webhooks/tests/test_job.py'
--- lib/lp/services/webhooks/tests/test_webhookjob.py 2015-08-04 00:11:46 +0000
+++ lib/lp/services/webhooks/tests/test_job.py 2015-08-04 06:04:54 +0000
@@ -241,9 +241,10 @@
layer = ZopelessDatabaseLayer
def makeAndRunJob(self, response_status=200, raises=None, mock=True,
- secret=None):
+ secret=None, active=True):
hook = self.factory.makeWebhook(
- delivery_url=u'http://example.com/ep', secret=secret)
+ delivery_url=u'http://example.com/ep', secret=secret,
+ active=active)
job = WebhookDeliveryJob.create(hook, payload={'foo': 'bar'})
client = MockWebhookClient(
@@ -286,6 +287,7 @@
pending=Equals(False),
successful=Equals(True),
date_sent=Not(Is(None)),
+ error_message=Is(None),
json_data=ContainsDict(
{'result': MatchesAll(
KeysEqual('request', 'response'),
@@ -327,6 +329,7 @@
pending=Equals(True),
successful=Equals(False),
date_sent=Not(Is(None)),
+ error_message=Equals('Bad HTTP response: 404'),
json_data=ContainsDict(
{'result': MatchesAll(
KeysEqual('request', 'response'),
@@ -349,6 +352,7 @@
pending=Equals(True),
successful=Equals(False),
date_sent=Not(Is(None)),
+ error_message=Equals('Connection error: Connection refused'),
json_data=ContainsDict(
{'result': MatchesAll(
KeysEqual('request', 'connection_error'),
@@ -373,12 +377,34 @@
pending=Equals(False),
successful=Is(None),
date_sent=Is(None),
+ error_message=Is(None),
json_data=Not(Contains('result'))))
self.assertEqual([], reqs)
self.assertEqual(1, len(oopses.oopses))
self.assertEqual(
'No webhook proxy configured.', oopses.oopses[0]['value'])
+ def test_run_inactive(self):
+ # A delivery for a webhook that has been deactivated immediately
+ # fails.
+ with CaptureOops() as oopses:
+ job, reqs = self.makeAndRunJob(
+ raises=requests.ConnectionError('Connection refused'),
+ active=False)
+ self.assertThat(
+ job,
+ MatchesStructure(
+ status=Equals(JobStatus.FAILED),
+ pending=Equals(False),
+ successful=Equals(False),
+ date_sent=Is(None),
+ error_message=Equals('Webhook deactivated'),
+ json_data=ContainsDict(
+ {'result': MatchesDict(
+ {'webhook_deactivated': Equals(True)})})))
+ self.assertEqual([], reqs)
+ self.assertEqual([], oopses.oopses)
+
def test_date_first_sent(self):
job, reqs = self.makeAndRunJob(response_status=404)
self.assertEqual(job.date_first_sent, job.date_sent)
@@ -513,6 +539,31 @@
self.assertEqual(True, self.runJob(job))
self.assertEqual(JobStatus.COMPLETED, job.status)
+ def test_manual_retry_with_reset(self):
+ # retry(reset=True) unsets date_first_sent so the automatic
+ # retries can be resumed. This can be useful for recovering from
+ # systemic errors the erroneously failed many deliveries.
+ hook = self.factory.makeWebhook()
+ job = WebhookDeliveryJob.create(hook, payload={'foo': 'bar'})
+ client = MockWebhookClient(response_status=404)
+ self.useFixture(ZopeUtilityFixture(client, IWebhookClient))
+
+ # Simulate a first attempt failure.
+ self.assertEqual(False, self.runJob(job))
+ self.assertEqual(JobStatus.WAITING, job.status)
+ self.assertIsNot(None, job.date_first_sent)
+
+ # A manual retry brings the scheduled start forward.
+ job.retry()
+ self.assertEqual(JobStatus.WAITING, job.status)
+ self.assertIsNot(None, job.date_first_sent)
+
+ # When reset=True, date_first_sent is unset to restart the 24
+ # hour auto-retry window.
+ job.retry(reset=True)
+ self.assertEqual(JobStatus.WAITING, job.status)
+ self.assertIs(None, job.date_first_sent)
+
class TestViaCronscript(TestCaseWithFactory):
=== renamed file 'lib/lp/services/webhooks/tests/test_webhook.py' => 'lib/lp/services/webhooks/tests/test_model.py'
--- lib/lp/services/webhooks/tests/test_webhook.py 2015-07-20 07:01:40 +0000
+++ lib/lp/services/webhooks/tests/test_model.py 2015-08-04 06:04:54 +0000
@@ -76,7 +76,7 @@
'active', 'date_created', 'date_last_modified', 'deliveries',
'delivery_url', 'destroySelf', 'event_types', 'getDelivery',
'id', 'ping', 'registrant', 'registrant_id', 'secret',
- 'target')),
+ 'setSecret', 'target')),
}
webhook = self.factory.makeWebhook()
checker = getChecker(webhook)
@@ -86,7 +86,8 @@
def test_set_permissions(self):
expected_set_permissions = {
'launchpad.View': set((
- 'active', 'delivery_url', 'event_types', 'registrant_id')),
+ 'active', 'delivery_url', 'event_types', 'registrant_id',
+ 'secret')),
}
webhook = self.factory.makeWebhook()
checker = getChecker(webhook)
=== modified file 'lib/lp/services/webhooks/tests/test_webservice.py'
--- lib/lp/services/webhooks/tests/test_webservice.py 2015-07-29 09:06:07 +0000
+++ lib/lp/services/webhooks/tests/test_webservice.py 2015-08-04 06:04:54 +0000
@@ -5,8 +5,10 @@
__metaclass__ = type
+from datetime import datetime
import json
+from pytz import utc
from testtools.matchers import (
ContainsDict,
Equals,
@@ -16,6 +18,7 @@
MatchesAll,
Not,
)
+from zope.security.proxy import removeSecurityProxy
from lp.services.features.testing import FeatureFixture
from lp.services.webapp.interfaces import OAuthPermission
@@ -136,6 +139,31 @@
self.webhook_url, api_version='devel')
self.assertEqual(404, get_response.status)
+ def test_setSecret(self):
+ with person_logged_in(self.owner):
+ self.assertIs(None, self.webhook.secret)
+ self.assertEqual(
+ 200,
+ self.webservice.named_post(
+ self.webhook_url, 'setSecret', secret='sekrit',
+ api_version='devel').status)
+ with person_logged_in(self.owner):
+ self.assertEqual(u'sekrit', self.webhook.secret)
+ self.assertEqual(
+ 200,
+ self.webservice.named_post(
+ self.webhook_url, 'setSecret', secret='shhh',
+ api_version='devel').status)
+ with person_logged_in(self.owner):
+ self.assertEqual(u'shhh', self.webhook.secret)
+ self.assertEqual(
+ 200,
+ self.webservice.named_post(
+ self.webhook_url, 'setSecret', secret=None,
+ api_version='devel').status)
+ with person_logged_in(self.owner):
+ self.assertIs(None, self.webhook.secret)
+
class TestWebhookDelivery(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
@@ -161,28 +189,46 @@
MatchesAll(
KeysEqual(
'date_created', 'date_first_sent', 'date_sent',
- 'http_etag', 'payload', 'pending', 'resource_type_link',
- 'self_link', 'successful', 'web_link', 'webhook_link'),
- ContainsDict(
- {'payload': Equals({'ping': True}),
+ 'error_message', 'http_etag', 'payload', 'pending',
+ 'resource_type_link', 'self_link', 'successful',
+ 'web_link', 'webhook_link'),
+ ContainsDict({
+ 'payload': Equals({'ping': True}),
'pending': Equals(True),
'successful': Is(None),
'date_created': Not(Is(None)),
- 'date_sent': Is(None)})))
+ 'date_sent': Is(None),
+ 'error_message': Is(None),
+ })))
def test_retry(self):
with person_logged_in(self.owner):
self.delivery.start()
+ removeSecurityProxy(self.delivery).json_data['date_first_sent'] = (
+ datetime.now(utc).isoformat())
self.delivery.fail()
representation = self.webservice.get(
self.delivery_url, api_version='devel').jsonBody()
self.assertFalse(representation['pending'])
+
+ # A normal retry just makes the job pending again.
response = self.webservice.named_post(
self.delivery_url, 'retry', api_version='devel')
self.assertEqual(200, response.status)
representation = self.webservice.get(
self.delivery_url, api_version='devel').jsonBody()
self.assertTrue(representation['pending'])
+ self.assertIsNot(None, representation['date_first_sent'])
+
+ # retry(reset=True) unsets date_first_sent as well, restarting
+ # the automatic retry window.
+ response = self.webservice.named_post(
+ self.delivery_url, 'retry', reset=True, api_version='devel')
+ self.assertEqual(200, response.status)
+ representation = self.webservice.get(
+ self.delivery_url, api_version='devel').jsonBody()
+ self.assertTrue(representation['pending'])
+ self.assertIs(None, representation['date_first_sent'])
class TestWebhookTarget(TestCaseWithFactory):
@@ -244,6 +290,21 @@
[(entry['delivery_url'], entry['event_types'], entry['active'])
for entry in representation['entries']])
+ def test_newWebhook_secret(self):
+ self.useFixture(FeatureFixture({'webhooks.new.enabled': 'true'}))
+ response = self.webservice.named_post(
+ self.target_url, 'newWebhook',
+ delivery_url='http://example.com/ep', event_types=['foo', 'bar'],
+ secret='sekrit', api_version='devel')
+ self.assertEqual(201, response.status)
+
+ # The secret is set, but cannot be read back through the API.
+ with person_logged_in(self.owner):
+ self.assertEqual(u'sekrit', self.target.webhooks.one().secret)
+ representation = self.webservice.get(
+ self.target_url + '/webhooks', api_version='devel').jsonBody()
+ self.assertNotIn(u'secret', representation['entries'][0])
+
def test_newWebhook_permissions(self):
self.useFixture(FeatureFixture({'webhooks.new.enabled': 'true'}))
webservice = LaunchpadWebServiceCaller()
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2015-08-04 00:19:36 +0000
+++ lib/lp/testing/factory.py 2015-08-04 06:04:54 +0000
@@ -4525,13 +4525,14 @@
return ProxyFactory(
LiveFSFile(livefsbuild=livefsbuild, libraryfile=libraryfile))
- def makeWebhook(self, target=None, delivery_url=None, secret=None):
+ def makeWebhook(self, target=None, delivery_url=None, secret=None,
+ active=True):
if target is None:
target = self.makeGitRepository()
if delivery_url is None:
delivery_url = self.getUniqueURL().decode('utf-8')
return getUtility(IWebhookSource).new(
- target, self.makePerson(), delivery_url, [], True, secret)
+ target, self.makePerson(), delivery_url, [], active, secret)
def makeSnap(self, registrant=None, owner=None, distroseries=None,
name=None, branch=None, git_ref=None,
Follow ups
