Merge lp:~cjwatson/launchpad/separate-build-notifications into lp:launchpad

Proposed by Colin Watson
Status: Merged
Merged at revision: 17636
Proposed branch: lp:~cjwatson/launchpad/separate-build-notifications
Merge into: lp:launchpad
Diff against target: 537 lines (+180/-108)
4 files modified
lib/lp/soyuz/doc/build-failedtoupload-workflow.txt (+17/-1)
lib/lp/soyuz/emailtemplates/build-notification.txt (+2/-0)
lib/lp/soyuz/model/binarypackagebuild.py (+18/-13)
lib/lp/soyuz/tests/test_build_notify.py (+143/-94)
To merge this branch: bzr merge lp:~cjwatson/launchpad/separate-build-notifications
Reviewer Review Type Date Requested Status
William Grant code Approve
Review via email: mp+264606@code.launchpad.net

Commit message

Add a rationale to build mail notifications.

Description of the change

Add a rationale to build mail notifications.

To post a comment you must log in.
Revision history for this message
William Grant (wgrant) :
review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/soyuz/doc/build-failedtoupload-workflow.txt'
2--- lib/lp/soyuz/doc/build-failedtoupload-workflow.txt 2015-02-17 07:37:36 +0000
3+++ lib/lp/soyuz/doc/build-failedtoupload-workflow.txt 2015-07-13 16:09:27 +0000
4@@ -67,7 +67,7 @@
5
6 Note that the generated notification contain the 'extra_info' content:
7
8- >>> build_notification = notifications.pop(0)
9+ >>> build_notification = notifications[0]
10
11 >>> build_notification['Subject']
12 '[Build #22] i386 build of cdrkit 1.0 in ubuntu breezy-autotest RELEASE'
13@@ -103,3 +103,19 @@
14 i386 build of cdrkit 1.0 in ubuntu breezy-autotest RELEASE
15 http://launchpad.dev/ubuntu/+source/cdrkit/1.0/+build/22
16 <BLANKLINE>
17+ You are receiving this email because you are a buildd administrator.
18+ <BLANKLINE>
19+
20+The other notifications are similar except for the footer.
21+
22+ >>> print notifications[1].get_payload()
23+ <BLANKLINE>
24+ ...
25+ You are receiving this email because you are a buildd administrator.
26+ <BLANKLINE>
27+ >>> print notifications[2].get_payload()
28+ <BLANKLINE>
29+ ...
30+ You are receiving this email because you created this version of this
31+ package.
32+ <BLANKLINE>
33
34=== modified file 'lib/lp/soyuz/emailtemplates/build-notification.txt'
35--- lib/lp/soyuz/emailtemplates/build-notification.txt 2011-12-18 23:10:57 +0000
36+++ lib/lp/soyuz/emailtemplates/build-notification.txt 2015-07-13 16:09:27 +0000
37@@ -18,3 +18,5 @@
38 --
39 %(build_title)s
40 %(build_url)s
41+
42+%(reason)s
43
44=== modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
45--- lib/lp/soyuz/model/binarypackagebuild.py 2015-07-08 16:05:11 +0000
46+++ lib/lp/soyuz/model/binarypackagebuild.py 2015-07-13 16:09:27 +0000
47@@ -17,6 +17,7 @@
48 attrgetter,
49 itemgetter,
50 )
51+import textwrap
52
53 import apt_pkg
54 import pytz
55@@ -702,7 +703,7 @@
56 def notify(self, extra_info=None):
57 """See `IPackageBuild`.
58
59- If config.buildmaster.build_notification is disable, simply
60+ If config.buildmaster.send_build_notification is disabled, simply
61 return.
62
63 If config.builddmaster.notify_owner is enabled and SPR.creator
64@@ -722,7 +723,7 @@
65 if self.status == BuildStatus.FULLYBUILT:
66 return
67
68- recipients = set()
69+ recipients = {}
70
71 fromaddress = format_address(
72 config.builddmaster.default_sender_name,
73@@ -764,12 +765,13 @@
74 # notified if they are the PPA owner or in the PPA team.
75 # (see bug 375757)
76 # Non-PPA notifications inform the creator regardless.
77- recipients = recipients.union(
78- get_contact_email_addresses(creator))
79+ for recipient in get_contact_email_addresses(creator):
80+ recipients.setdefault(
81+ recipient, "you created this version of this package")
82 dsc_key = self.source_package_release.dscsigningkey
83 if dsc_key:
84- recipients = recipients.union(
85- get_contact_email_addresses(dsc_key.owner))
86+ for recipient in get_contact_email_addresses(dsc_key.owner):
87+ recipients.setdefault(recipient, "you signed this package")
88
89 # Modify notification contents according to the targeted archive.
90 # 'Archive Tag', 'Subject' and 'Source URL' are customized for PPA.
91@@ -780,12 +782,13 @@
92 subject = "[Build #%d] %s" % (self.id, self.title)
93 if not self.archive.is_ppa:
94 buildd_admins = getUtility(ILaunchpadCelebrities).buildd_admin
95- recipients = recipients.union(
96- get_contact_email_addresses(buildd_admins))
97+ for recipient in get_contact_email_addresses(buildd_admins):
98+ recipients.setdefault(
99+ recipient, "you are a buildd administrator")
100 source_url = canonical_url(self.distributionsourcepackagerelease)
101 else:
102- recipients = recipients.union(
103- get_contact_email_addresses(self.archive.owner))
104+ for recipient in get_contact_email_addresses(self.archive.owner):
105+ recipients.setdefault(recipient, "you own this archive")
106 # For PPAs we run the risk of having no available contact_address,
107 # for instance, when both, SPR.creator and Archive.owner have
108 # not enabled it.
109@@ -803,7 +806,7 @@
110 # pocket build. We don't build SECURITY yet :(
111
112 # XXX cprov 2006-08-02: find out a way to glue parameters reported
113- # with the state in the build worflow, maybe by having an
114+ # with the state in the build workflow, maybe by having an
115 # IBuild.statusReport property, which could also be used in the
116 # respective page template.
117 if self.status in [
118@@ -852,9 +855,11 @@
119 'archive_tag': self.archive.reference,
120 'component_tag': self.current_component.name,
121 }
122- message = template % replacements
123
124- for toaddress in recipients:
125+ for toaddress, reason in recipients.items():
126+ replacements['reason'] = textwrap.fill(
127+ 'You are receiving this email because %s.' % reason, width=72)
128+ message = template % replacements
129 simple_sendmail(
130 fromaddress, toaddress, subject, message,
131 headers=extra_headers)
132
133=== modified file 'lib/lp/soyuz/tests/test_build_notify.py'
134--- lib/lp/soyuz/tests/test_build_notify.py 2015-04-20 15:59:52 +0000
135+++ lib/lp/soyuz/tests/test_build_notify.py 2015-07-13 16:09:27 +0000
136@@ -4,6 +4,7 @@
137 __metaclass__ = type
138
139 from datetime import timedelta
140+from operator import itemgetter
141 from textwrap import dedent
142
143 from zope.component import getUtility
144@@ -27,6 +28,18 @@
145 from lp.testing.sampledata import ADMIN_EMAIL
146
147
148+REASONS = {
149+ "creator": (
150+ "You are receiving this email because you created this version of "
151+ "this\npackage."),
152+ "signer": "You are receiving this email because you signed this package.",
153+ "buildd-admin": (
154+ "You are receiving this email because you are a buildd "
155+ "administrator."),
156+ "owner": "You are receiving this email because you own this archive.",
157+ }
158+
159+
160 class TestBuildNotify(TestCaseWithFactory):
161
162 layer = LaunchpadFunctionalLayer
163@@ -51,6 +64,7 @@
164 'launchpad-buildd-admins')
165 self.buildd_admins_email = []
166 with person_logged_in(self.admin):
167+ self.ppa_owner_email = self.ppa.owner.preferredemail.email
168 self.publisher = SoyuzTestPublisher()
169 self.publisher.prepareBreezyAutotest()
170 self.distroseries.nominatedarchindep = self.das
171@@ -83,21 +97,22 @@
172 build.buildqueue_record.builder = self.builder
173 self.builds.append(build)
174
175- def _assert_mail_is_correct(self, build, notification, ppa=False):
176+ def _assert_mail_is_correct(self, build, notification, recipient, reason,
177+ ppa=False):
178 # Assert that the mail sent (which is in notification), matches
179 # the data from the build
180- self.assertEquals('test@example.com',
181- notification['X-Creator-Recipient'])
182- self.assertEquals(
183+ self.assertEqual(recipient, notification['To'])
184+ self.assertEqual(
185+ 'test@example.com', notification['X-Creator-Recipient'])
186+ self.assertEqual(
187 self.das.architecturetag, notification['X-Launchpad-Build-Arch'])
188- self.assertEquals(
189- 'main', notification['X-Launchpad-Build-Component'])
190- self.assertEquals(
191+ self.assertEqual('main', notification['X-Launchpad-Build-Component'])
192+ self.assertEqual(
193 build.status.name, notification['X-Launchpad-Build-State'])
194- self.assertEquals(
195+ self.assertEqual(
196 build.archive.reference, notification['X-Launchpad-Archive'])
197 if ppa and build.archive.distribution.name == u'ubuntu':
198- self.assertEquals(
199+ self.assertEqual(
200 get_ppa_reference(self.ppa), notification['X-Launchpad-PPA'])
201 body = notification.get_payload(decode=True)
202 build_log = 'None'
203@@ -105,10 +120,10 @@
204 source = 'not available'
205 else:
206 source = canonical_url(build.distributionsourcepackagerelease)
207- builder = canonical_url(build.builder)
208 if build.status == BuildStatus.BUILDING:
209 duration = 'not finished'
210 build_log = 'see builder page'
211+ builder = canonical_url(build.builder)
212 elif (
213 build.status == BuildStatus.SUPERSEDED or
214 build.status == BuildStatus.NEEDSBUILD):
215@@ -122,6 +137,7 @@
216 else:
217 duration = DurationFormatterAPI(
218 build.duration).approximateduration()
219+ builder = canonical_url(build.builder)
220 expected_body = dedent("""
221 * Source Package: %s
222 * Version: %s
223@@ -147,59 +163,61 @@
224 build.source_package_release.version, self.das.architecturetag,
225 build.archive.reference, build.status.title, duration, build_log,
226 builder, source, build.title, canonical_url(build)))
227- self.assertEquals(expected_body, body)
228-
229- def test_notify_buildd_admins(self):
230- # A build will cause an e-mail to be sent out to the buildd-admins,
231- # for primary archive builds.
232- self.create_builds(self.archive)
233- build = self.builds[BuildStatus.FAILEDTOBUILD.value]
234- build.notify()
235- expected_emails = self.buildd_admins_email + ['test@example.com']
236- notifications = pop_notifications()
237- actual_emails = [n['To'] for n in notifications]
238- self.assertEquals(expected_emails, actual_emails)
239-
240- def test_ppa_does_not_notify_buildd_admins(self):
241- # A build for a PPA does not notify the buildd admins.
242- self.create_builds(self.ppa)
243- build = self.builds[BuildStatus.FAILEDTOBUILD.value]
244- build.notify()
245- notifications = pop_notifications()
246- # An e-mail is sent to the archive owner, as well as the creator
247- self.assertEquals(2, len(notifications))
248+ expected_body += "\n" + REASONS[reason] + "\n"
249+ self.assertEqual(expected_body, body)
250+
251+ def _assert_mails_are_correct(self, build, reasons, ppa=False):
252+ notifications = pop_notifications()
253+ reasons = sorted(reasons, key=itemgetter(0))
254+ for notification, (recipient, reason) in zip(notifications, reasons):
255+ self._assert_mail_is_correct(
256+ build, notification, recipient, reason, ppa=ppa)
257
258 def test_notify_failed_to_build(self):
259- # An e-mail is sent to the source package creator on build failures.
260+ # For primary archive builds, a build failure notifies the buildd
261+ # admins and the source package creator.
262 self.create_builds(self.archive)
263 build = self.builds[BuildStatus.FAILEDTOBUILD.value]
264 build.notify()
265- notification = pop_notifications()[1]
266- self._assert_mail_is_correct(build, notification)
267+ expected_reasons = [
268+ (email, "buildd-admin") for email in self.buildd_admins_email]
269+ expected_reasons.append(("test@example.com", "creator"))
270+ self._assert_mails_are_correct(build, expected_reasons)
271
272 def test_notify_failed_to_build_ppa(self):
273- # An e-mail is sent to the source package creator on build failures.
274- self.create_builds(archive=self.ppa)
275+ # For PPA builds, a build failure notifies the source package signer
276+ # and the archive owner, but not the buildd admins.
277+ self.create_builds(self.ppa)
278 build = self.builds[BuildStatus.FAILEDTOBUILD.value]
279 build.notify()
280- notification = pop_notifications()[1]
281- self._assert_mail_is_correct(build, notification, ppa=True)
282+ expected_reasons = [
283+ ("test@example.com", "signer"),
284+ (self.ppa_owner_email, "owner"),
285+ ]
286+ self._assert_mails_are_correct(build, expected_reasons, ppa=True)
287
288 def test_notify_needs_building(self):
289- # We can notify the creator when the build is needing to be built.
290+ # We can notify the creator and buildd admins when a build needs to
291+ # be built.
292 self.create_builds(self.archive)
293 build = self.builds[BuildStatus.NEEDSBUILD.value]
294 build.notify()
295- notification = pop_notifications()[1]
296- self._assert_mail_is_correct(build, notification)
297+ expected_reasons = [
298+ (email, "buildd-admin") for email in self.buildd_admins_email]
299+ expected_reasons.append(("test@example.com", "creator"))
300+ self._assert_mails_are_correct(build, expected_reasons)
301
302 def test_notify_needs_building_ppa(self):
303- # We can notify the creator when the build is needing to be built.
304+ # We can notify the signer and the archive owner when a build needs
305+ # to be built.
306 self.create_builds(self.ppa)
307 build = self.builds[BuildStatus.NEEDSBUILD.value]
308 build.notify()
309- notification = pop_notifications()[1]
310- self._assert_mail_is_correct(build, notification, ppa=True)
311+ expected_reasons = [
312+ ("test@example.com", "signer"),
313+ (self.ppa_owner_email, "owner"),
314+ ]
315+ self._assert_mails_are_correct(build, expected_reasons, ppa=True)
316
317 def test_notify_successfully_built(self):
318 # Successful builds don't notify anyone.
319@@ -209,90 +227,121 @@
320 self.assertEqual([], pop_notifications())
321
322 def test_notify_dependency_wait(self):
323- # We can notify the creator when the build can't find a dependency.
324+ # We can notify the creator and buildd admins when a build can't
325+ # find a dependency.
326 self.create_builds(self.archive)
327 build = self.builds[BuildStatus.MANUALDEPWAIT.value]
328 build.notify()
329- notification = pop_notifications()[1]
330- self._assert_mail_is_correct(build, notification)
331+ expected_reasons = [
332+ (email, "buildd-admin") for email in self.buildd_admins_email]
333+ expected_reasons.append(("test@example.com", "creator"))
334+ self._assert_mails_are_correct(build, expected_reasons)
335
336 def test_notify_dependency_wait_ppa(self):
337- # We can notify the creator when the build can't find a dependency.
338+ # We can notify the signer and the archive owner when the build
339+ # can't find a dependency.
340 self.create_builds(self.ppa)
341 build = self.builds[BuildStatus.MANUALDEPWAIT.value]
342 build.notify()
343- notification = pop_notifications()[1]
344- self._assert_mail_is_correct(build, notification, ppa=True)
345+ expected_reasons = [
346+ ("test@example.com", "signer"),
347+ (self.ppa_owner_email, "owner"),
348+ ]
349+ self._assert_mails_are_correct(build, expected_reasons, ppa=True)
350
351 def test_notify_chroot_problem(self):
352- # We can notify the creator when the builder the build attempted to
353- # be built on has an internal problem.
354+ # We can notify the creator and buildd admins when the builder a
355+ # build attempted to be built on has an internal problem.
356 self.create_builds(self.archive)
357 build = self.builds[BuildStatus.CHROOTWAIT.value]
358 build.notify()
359- notification = pop_notifications()[1]
360- self._assert_mail_is_correct(build, notification)
361+ expected_reasons = [
362+ (email, "buildd-admin") for email in self.buildd_admins_email]
363+ expected_reasons.append(("test@example.com", "creator"))
364+ self._assert_mails_are_correct(build, expected_reasons)
365
366 def test_notify_chroot_problem_ppa(self):
367- # We can notify the creator when the builder the build attempted to
368- # be built on has an internal problem.
369+ # We can notify the signer and the archive owner when the builder a
370+ # build attempted to be built on has an internal problem.
371 self.create_builds(self.ppa)
372 build = self.builds[BuildStatus.CHROOTWAIT.value]
373 build.notify()
374- notification = pop_notifications()[1]
375- self._assert_mail_is_correct(build, notification, ppa=True)
376+ expected_reasons = [
377+ ("test@example.com", "signer"),
378+ (self.ppa_owner_email, "owner"),
379+ ]
380+ self._assert_mails_are_correct(build, expected_reasons, ppa=True)
381
382 def test_notify_build_for_superseded_source(self):
383- # We can notify the creator when the source package had a newer
384- # version uploaded before this build had a chance to be dispatched.
385+ # We can notify the creator and buildd admins when the source
386+ # package had a newer version uploaded before this build had a
387+ # chance to be dispatched.
388 self.create_builds(self.archive)
389 build = self.builds[BuildStatus.SUPERSEDED.value]
390 build.notify()
391- notification = pop_notifications()[1]
392- self._assert_mail_is_correct(build, notification)
393+ expected_reasons = [
394+ (email, "buildd-admin") for email in self.buildd_admins_email]
395+ expected_reasons.append(("test@example.com", "creator"))
396+ self._assert_mails_are_correct(build, expected_reasons)
397
398 def test_notify_build_for_superseded_source_ppa(self):
399- # We can notify the creator when the source package had a newer
400- # version uploaded before this build had a chance to be dispatched.
401+ # We can notify the signer and the archive owner when the source
402+ # package had a newer version uploaded before this build had a
403+ # chance to be dispatched.
404 self.create_builds(self.ppa)
405 build = self.builds[BuildStatus.SUPERSEDED.value]
406 build.notify()
407- notification = pop_notifications()[1]
408- self._assert_mail_is_correct(build, notification, ppa=True)
409+ expected_reasons = [
410+ ("test@example.com", "signer"),
411+ (self.ppa_owner_email, "owner"),
412+ ]
413+ self._assert_mails_are_correct(build, expected_reasons, ppa=True)
414
415 def test_notify_currently_building(self):
416- # We can notify the creator when the build is currently building.
417+ # We can notify the creator and buildd admins when the build is
418+ # currently building.
419 self.create_builds(self.archive)
420 build = self.builds[BuildStatus.BUILDING.value]
421 build.notify()
422- notification = pop_notifications()[1]
423- self._assert_mail_is_correct(build, notification)
424+ expected_reasons = [
425+ (email, "buildd-admin") for email in self.buildd_admins_email]
426+ expected_reasons.append(("test@example.com", "creator"))
427+ self._assert_mails_are_correct(build, expected_reasons)
428
429 def test_notify_currently_building_ppa(self):
430- # We can notify the creator when the build is currently building.
431+ # We can notify the signer and the archive owner when the build is
432+ # currently building.
433 self.create_builds(self.ppa)
434 build = self.builds[BuildStatus.BUILDING.value]
435 build.notify()
436- notification = pop_notifications()[1]
437- self._assert_mail_is_correct(build, notification, ppa=True)
438+ expected_reasons = [
439+ ("test@example.com", "signer"),
440+ (self.ppa_owner_email, "owner"),
441+ ]
442+ self._assert_mails_are_correct(build, expected_reasons, ppa=True)
443
444 def test_notify_uploading_build(self):
445- # We can notify the creator when the build has completed, and binary
446- # packages are being uploaded by the builder.
447+ # We can notify the creator and buildd admins when the build has
448+ # completed, and binary packages are being uploaded by the builder.
449 self.create_builds(self.archive)
450 build = self.builds[BuildStatus.UPLOADING.value]
451 build.notify()
452- notification = pop_notifications()[1]
453- self._assert_mail_is_correct(build, notification)
454+ expected_reasons = [
455+ (email, "buildd-admin") for email in self.buildd_admins_email]
456+ expected_reasons.append(("test@example.com", "creator"))
457+ self._assert_mails_are_correct(build, expected_reasons)
458
459 def test_notify_uploading_build_ppa(self):
460- # We can notify the creator when the build has completed, and binary
461- # packages are being uploaded by the builder.
462+ # We can notify the signer and the archive owner when the build has
463+ # completed, and binary packages are being uploaded by the builder.
464 self.create_builds(self.ppa)
465 build = self.builds[BuildStatus.UPLOADING.value]
466 build.notify()
467- notification = pop_notifications()[1]
468- self._assert_mail_is_correct(build, notification, ppa=True)
469+ expected_reasons = [
470+ ("test@example.com", "signer"),
471+ (self.ppa_owner_email, "owner"),
472+ ]
473+ self._assert_mails_are_correct(build, expected_reasons, ppa=True)
474
475 def test_copied_into_ppa_does_not_spam(self):
476 # When a package is copied into a PPA, we don't send mail to the
477@@ -304,10 +353,10 @@
478 self.distroseries, PackagePublishingPocket.RELEASE, self.ppa)
479 [ppa_build] = ppa_spph.createMissingBuilds()
480 ppa_build.notify()
481- notifications = pop_notifications()
482- self.assertEquals(1, len(notifications))
483+ self._assert_mails_are_correct(
484+ ppa_build, [(self.ppa_owner_email, "owner")], ppa=True)
485
486- def test_notify_owner_supresses_mail(self):
487+ def test_notify_owner_suppresses_mail(self):
488 # When the 'notify_owner' config option is False, we don't send mail
489 # to the owner of the SPR.
490 self.create_builds(self.archive)
491@@ -319,13 +368,13 @@
492 """)
493 config.push('notify_owner', notify_owner)
494 build.notify()
495- notifications = pop_notifications()
496- actual_emails = [n['To'] for n in notifications]
497- self.assertEquals(self.buildd_admins_email, actual_emails)
498+ self._assert_mails_are_correct(
499+ build,
500+ [(email, "buildd-admin") for email in self.buildd_admins_email])
501 # And undo what we just did.
502 config.pop('notify_owner')
503
504- def test_build_notification_supresses_mail(self):
505+ def test_build_notification_suppresses_mail(self):
506 # When the 'build_notification' config option is False, we don't
507 # send any mail at all.
508 self.create_builds(self.archive)
509@@ -337,12 +386,12 @@
510 config.push('send_build_notification', send_build_notification)
511 build.notify()
512 notifications = pop_notifications()
513- self.assertEquals(0, len(notifications))
514+ self.assertEqual(0, len(notifications))
515 # And undo what we just did.
516 config.pop('send_build_notification')
517
518 def test_sponsored_upload_notification(self):
519- # If the signing key is different to the creator, they are both
520+ # If the signing key is different from the creator, they are both
521 # notified.
522 sponsor = self.factory.makePerson('sponsor@example.com')
523 key = self.factory.makeGPGKey(owner=sponsor)
524@@ -352,8 +401,8 @@
525 # Push past the security proxy
526 removeSecurityProxy(spr).dscsigningkey = key
527 build.notify()
528- notifications = pop_notifications()
529- expected_emails = self.buildd_admins_email + [
530- 'sponsor@example.com', 'test@example.com']
531- actual_emails = [n['To'] for n in notifications]
532- self.assertEquals(expected_emails, actual_emails)
533+ expected_reasons = [
534+ (email, "buildd-admin") for email in self.buildd_admins_email]
535+ expected_reasons.append(("test@example.com", "creator"))
536+ expected_reasons.append(("sponsor@example.com", "signer"))
537+ self._assert_mails_are_correct(build, expected_reasons)