launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #18999
[Merge] lp:~cjwatson/launchpad/separate-build-notifications into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/separate-build-notifications into lp:launchpad.
Commit message:
Add a rationale to build mail notifications.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #410893 in Launchpad itself: "PPA Buildd emails are missing a rationale message"
https://bugs.launchpad.net/launchpad/+bug/410893
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/separate-build-notifications/+merge/264606
Add a rationale to build mail notifications.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/separate-build-notifications into lp:launchpad.
=== modified file 'lib/lp/soyuz/doc/build-failedtoupload-workflow.txt'
--- lib/lp/soyuz/doc/build-failedtoupload-workflow.txt 2015-02-17 07:37:36 +0000
+++ lib/lp/soyuz/doc/build-failedtoupload-workflow.txt 2015-07-13 16:09:27 +0000
@@ -67,7 +67,7 @@
Note that the generated notification contain the 'extra_info' content:
- >>> build_notification = notifications.pop(0)
+ >>> build_notification = notifications[0]
>>> build_notification['Subject']
'[Build #22] i386 build of cdrkit 1.0 in ubuntu breezy-autotest RELEASE'
@@ -103,3 +103,19 @@
i386 build of cdrkit 1.0 in ubuntu breezy-autotest RELEASE
http://launchpad.dev/ubuntu/+source/cdrkit/1.0/+build/22
<BLANKLINE>
+ You are receiving this email because you are a buildd administrator.
+ <BLANKLINE>
+
+The other notifications are similar except for the footer.
+
+ >>> print notifications[1].get_payload()
+ <BLANKLINE>
+ ...
+ You are receiving this email because you are a buildd administrator.
+ <BLANKLINE>
+ >>> print notifications[2].get_payload()
+ <BLANKLINE>
+ ...
+ You are receiving this email because you created this version of this
+ package.
+ <BLANKLINE>
=== modified file 'lib/lp/soyuz/emailtemplates/build-notification.txt'
--- lib/lp/soyuz/emailtemplates/build-notification.txt 2011-12-18 23:10:57 +0000
+++ lib/lp/soyuz/emailtemplates/build-notification.txt 2015-07-13 16:09:27 +0000
@@ -18,3 +18,5 @@
--
%(build_title)s
%(build_url)s
+
+%(reason)s
=== modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
--- lib/lp/soyuz/model/binarypackagebuild.py 2015-07-08 16:05:11 +0000
+++ lib/lp/soyuz/model/binarypackagebuild.py 2015-07-13 16:09:27 +0000
@@ -17,6 +17,7 @@
attrgetter,
itemgetter,
)
+import textwrap
import apt_pkg
import pytz
@@ -702,7 +703,7 @@
def notify(self, extra_info=None):
"""See `IPackageBuild`.
- If config.buildmaster.build_notification is disable, simply
+ If config.buildmaster.send_build_notification is disabled, simply
return.
If config.builddmaster.notify_owner is enabled and SPR.creator
@@ -722,7 +723,7 @@
if self.status == BuildStatus.FULLYBUILT:
return
- recipients = set()
+ recipients = {}
fromaddress = format_address(
config.builddmaster.default_sender_name,
@@ -764,12 +765,13 @@
# notified if they are the PPA owner or in the PPA team.
# (see bug 375757)
# Non-PPA notifications inform the creator regardless.
- recipients = recipients.union(
- get_contact_email_addresses(creator))
+ for recipient in get_contact_email_addresses(creator):
+ recipients.setdefault(
+ recipient, "you created this version of this package")
dsc_key = self.source_package_release.dscsigningkey
if dsc_key:
- recipients = recipients.union(
- get_contact_email_addresses(dsc_key.owner))
+ for recipient in get_contact_email_addresses(dsc_key.owner):
+ recipients.setdefault(recipient, "you signed this package")
# Modify notification contents according to the targeted archive.
# 'Archive Tag', 'Subject' and 'Source URL' are customized for PPA.
@@ -780,12 +782,13 @@
subject = "[Build #%d] %s" % (self.id, self.title)
if not self.archive.is_ppa:
buildd_admins = getUtility(ILaunchpadCelebrities).buildd_admin
- recipients = recipients.union(
- get_contact_email_addresses(buildd_admins))
+ for recipient in get_contact_email_addresses(buildd_admins):
+ recipients.setdefault(
+ recipient, "you are a buildd administrator")
source_url = canonical_url(self.distributionsourcepackagerelease)
else:
- recipients = recipients.union(
- get_contact_email_addresses(self.archive.owner))
+ for recipient in get_contact_email_addresses(self.archive.owner):
+ recipients.setdefault(recipient, "you own this archive")
# For PPAs we run the risk of having no available contact_address,
# for instance, when both, SPR.creator and Archive.owner have
# not enabled it.
@@ -803,7 +806,7 @@
# pocket build. We don't build SECURITY yet :(
# XXX cprov 2006-08-02: find out a way to glue parameters reported
- # with the state in the build worflow, maybe by having an
+ # with the state in the build workflow, maybe by having an
# IBuild.statusReport property, which could also be used in the
# respective page template.
if self.status in [
@@ -852,9 +855,11 @@
'archive_tag': self.archive.reference,
'component_tag': self.current_component.name,
}
- message = template % replacements
- for toaddress in recipients:
+ for toaddress, reason in recipients.items():
+ replacements['reason'] = textwrap.fill(
+ 'You are receiving this email because %s.' % reason, width=72)
+ message = template % replacements
simple_sendmail(
fromaddress, toaddress, subject, message,
headers=extra_headers)
=== modified file 'lib/lp/soyuz/tests/test_build_notify.py'
--- lib/lp/soyuz/tests/test_build_notify.py 2015-04-20 15:59:52 +0000
+++ lib/lp/soyuz/tests/test_build_notify.py 2015-07-13 16:09:27 +0000
@@ -4,6 +4,7 @@
__metaclass__ = type
from datetime import timedelta
+from operator import itemgetter
from textwrap import dedent
from zope.component import getUtility
@@ -27,6 +28,18 @@
from lp.testing.sampledata import ADMIN_EMAIL
+REASONS = {
+ "creator": (
+ "You are receiving this email because you created this version of "
+ "this\npackage."),
+ "signer": "You are receiving this email because you signed this package.",
+ "buildd-admin": (
+ "You are receiving this email because you are a buildd "
+ "administrator."),
+ "owner": "You are receiving this email because you own this archive.",
+ }
+
+
class TestBuildNotify(TestCaseWithFactory):
layer = LaunchpadFunctionalLayer
@@ -51,6 +64,7 @@
'launchpad-buildd-admins')
self.buildd_admins_email = []
with person_logged_in(self.admin):
+ self.ppa_owner_email = self.ppa.owner.preferredemail.email
self.publisher = SoyuzTestPublisher()
self.publisher.prepareBreezyAutotest()
self.distroseries.nominatedarchindep = self.das
@@ -83,21 +97,22 @@
build.buildqueue_record.builder = self.builder
self.builds.append(build)
- def _assert_mail_is_correct(self, build, notification, ppa=False):
+ def _assert_mail_is_correct(self, build, notification, recipient, reason,
+ ppa=False):
# Assert that the mail sent (which is in notification), matches
# the data from the build
- self.assertEquals('test@xxxxxxxxxxx',
- notification['X-Creator-Recipient'])
- self.assertEquals(
+ self.assertEqual(recipient, notification['To'])
+ self.assertEqual(
+ 'test@xxxxxxxxxxx', notification['X-Creator-Recipient'])
+ self.assertEqual(
self.das.architecturetag, notification['X-Launchpad-Build-Arch'])
- self.assertEquals(
- 'main', notification['X-Launchpad-Build-Component'])
- self.assertEquals(
+ self.assertEqual('main', notification['X-Launchpad-Build-Component'])
+ self.assertEqual(
build.status.name, notification['X-Launchpad-Build-State'])
- self.assertEquals(
+ self.assertEqual(
build.archive.reference, notification['X-Launchpad-Archive'])
if ppa and build.archive.distribution.name == u'ubuntu':
- self.assertEquals(
+ self.assertEqual(
get_ppa_reference(self.ppa), notification['X-Launchpad-PPA'])
body = notification.get_payload(decode=True)
build_log = 'None'
@@ -105,10 +120,10 @@
source = 'not available'
else:
source = canonical_url(build.distributionsourcepackagerelease)
- builder = canonical_url(build.builder)
if build.status == BuildStatus.BUILDING:
duration = 'not finished'
build_log = 'see builder page'
+ builder = canonical_url(build.builder)
elif (
build.status == BuildStatus.SUPERSEDED or
build.status == BuildStatus.NEEDSBUILD):
@@ -122,6 +137,7 @@
else:
duration = DurationFormatterAPI(
build.duration).approximateduration()
+ builder = canonical_url(build.builder)
expected_body = dedent("""
* Source Package: %s
* Version: %s
@@ -147,59 +163,61 @@
build.source_package_release.version, self.das.architecturetag,
build.archive.reference, build.status.title, duration, build_log,
builder, source, build.title, canonical_url(build)))
- self.assertEquals(expected_body, body)
-
- def test_notify_buildd_admins(self):
- # A build will cause an e-mail to be sent out to the buildd-admins,
- # for primary archive builds.
- self.create_builds(self.archive)
- build = self.builds[BuildStatus.FAILEDTOBUILD.value]
- build.notify()
- expected_emails = self.buildd_admins_email + ['test@xxxxxxxxxxx']
- notifications = pop_notifications()
- actual_emails = [n['To'] for n in notifications]
- self.assertEquals(expected_emails, actual_emails)
-
- def test_ppa_does_not_notify_buildd_admins(self):
- # A build for a PPA does not notify the buildd admins.
- self.create_builds(self.ppa)
- build = self.builds[BuildStatus.FAILEDTOBUILD.value]
- build.notify()
- notifications = pop_notifications()
- # An e-mail is sent to the archive owner, as well as the creator
- self.assertEquals(2, len(notifications))
+ expected_body += "\n" + REASONS[reason] + "\n"
+ self.assertEqual(expected_body, body)
+
+ def _assert_mails_are_correct(self, build, reasons, ppa=False):
+ notifications = pop_notifications()
+ reasons = sorted(reasons, key=itemgetter(0))
+ for notification, (recipient, reason) in zip(notifications, reasons):
+ self._assert_mail_is_correct(
+ build, notification, recipient, reason, ppa=ppa)
def test_notify_failed_to_build(self):
- # An e-mail is sent to the source package creator on build failures.
+ # For primary archive builds, a build failure notifies the buildd
+ # admins and the source package creator.
self.create_builds(self.archive)
build = self.builds[BuildStatus.FAILEDTOBUILD.value]
build.notify()
- notification = pop_notifications()[1]
- self._assert_mail_is_correct(build, notification)
+ expected_reasons = [
+ (email, "buildd-admin") for email in self.buildd_admins_email]
+ expected_reasons.append(("test@xxxxxxxxxxx", "creator"))
+ self._assert_mails_are_correct(build, expected_reasons)
def test_notify_failed_to_build_ppa(self):
- # An e-mail is sent to the source package creator on build failures.
- self.create_builds(archive=self.ppa)
+ # For PPA builds, a build failure notifies the source package signer
+ # and the archive owner, but not the buildd admins.
+ self.create_builds(self.ppa)
build = self.builds[BuildStatus.FAILEDTOBUILD.value]
build.notify()
- notification = pop_notifications()[1]
- self._assert_mail_is_correct(build, notification, ppa=True)
+ expected_reasons = [
+ ("test@xxxxxxxxxxx", "signer"),
+ (self.ppa_owner_email, "owner"),
+ ]
+ self._assert_mails_are_correct(build, expected_reasons, ppa=True)
def test_notify_needs_building(self):
- # We can notify the creator when the build is needing to be built.
+ # We can notify the creator and buildd admins when a build needs to
+ # be built.
self.create_builds(self.archive)
build = self.builds[BuildStatus.NEEDSBUILD.value]
build.notify()
- notification = pop_notifications()[1]
- self._assert_mail_is_correct(build, notification)
+ expected_reasons = [
+ (email, "buildd-admin") for email in self.buildd_admins_email]
+ expected_reasons.append(("test@xxxxxxxxxxx", "creator"))
+ self._assert_mails_are_correct(build, expected_reasons)
def test_notify_needs_building_ppa(self):
- # We can notify the creator when the build is needing to be built.
+ # We can notify the signer and the archive owner when a build needs
+ # to be built.
self.create_builds(self.ppa)
build = self.builds[BuildStatus.NEEDSBUILD.value]
build.notify()
- notification = pop_notifications()[1]
- self._assert_mail_is_correct(build, notification, ppa=True)
+ expected_reasons = [
+ ("test@xxxxxxxxxxx", "signer"),
+ (self.ppa_owner_email, "owner"),
+ ]
+ self._assert_mails_are_correct(build, expected_reasons, ppa=True)
def test_notify_successfully_built(self):
# Successful builds don't notify anyone.
@@ -209,90 +227,121 @@
self.assertEqual([], pop_notifications())
def test_notify_dependency_wait(self):
- # We can notify the creator when the build can't find a dependency.
+ # We can notify the creator and buildd admins when a build can't
+ # find a dependency.
self.create_builds(self.archive)
build = self.builds[BuildStatus.MANUALDEPWAIT.value]
build.notify()
- notification = pop_notifications()[1]
- self._assert_mail_is_correct(build, notification)
+ expected_reasons = [
+ (email, "buildd-admin") for email in self.buildd_admins_email]
+ expected_reasons.append(("test@xxxxxxxxxxx", "creator"))
+ self._assert_mails_are_correct(build, expected_reasons)
def test_notify_dependency_wait_ppa(self):
- # We can notify the creator when the build can't find a dependency.
+ # We can notify the signer and the archive owner when the build
+ # can't find a dependency.
self.create_builds(self.ppa)
build = self.builds[BuildStatus.MANUALDEPWAIT.value]
build.notify()
- notification = pop_notifications()[1]
- self._assert_mail_is_correct(build, notification, ppa=True)
+ expected_reasons = [
+ ("test@xxxxxxxxxxx", "signer"),
+ (self.ppa_owner_email, "owner"),
+ ]
+ self._assert_mails_are_correct(build, expected_reasons, ppa=True)
def test_notify_chroot_problem(self):
- # We can notify the creator when the builder the build attempted to
- # be built on has an internal problem.
+ # We can notify the creator and buildd admins when the builder a
+ # build attempted to be built on has an internal problem.
self.create_builds(self.archive)
build = self.builds[BuildStatus.CHROOTWAIT.value]
build.notify()
- notification = pop_notifications()[1]
- self._assert_mail_is_correct(build, notification)
+ expected_reasons = [
+ (email, "buildd-admin") for email in self.buildd_admins_email]
+ expected_reasons.append(("test@xxxxxxxxxxx", "creator"))
+ self._assert_mails_are_correct(build, expected_reasons)
def test_notify_chroot_problem_ppa(self):
- # We can notify the creator when the builder the build attempted to
- # be built on has an internal problem.
+ # We can notify the signer and the archive owner when the builder a
+ # build attempted to be built on has an internal problem.
self.create_builds(self.ppa)
build = self.builds[BuildStatus.CHROOTWAIT.value]
build.notify()
- notification = pop_notifications()[1]
- self._assert_mail_is_correct(build, notification, ppa=True)
+ expected_reasons = [
+ ("test@xxxxxxxxxxx", "signer"),
+ (self.ppa_owner_email, "owner"),
+ ]
+ self._assert_mails_are_correct(build, expected_reasons, ppa=True)
def test_notify_build_for_superseded_source(self):
- # We can notify the creator when the source package had a newer
- # version uploaded before this build had a chance to be dispatched.
+ # We can notify the creator and buildd admins when the source
+ # package had a newer version uploaded before this build had a
+ # chance to be dispatched.
self.create_builds(self.archive)
build = self.builds[BuildStatus.SUPERSEDED.value]
build.notify()
- notification = pop_notifications()[1]
- self._assert_mail_is_correct(build, notification)
+ expected_reasons = [
+ (email, "buildd-admin") for email in self.buildd_admins_email]
+ expected_reasons.append(("test@xxxxxxxxxxx", "creator"))
+ self._assert_mails_are_correct(build, expected_reasons)
def test_notify_build_for_superseded_source_ppa(self):
- # We can notify the creator when the source package had a newer
- # version uploaded before this build had a chance to be dispatched.
+ # We can notify the signer and the archive owner when the source
+ # package had a newer version uploaded before this build had a
+ # chance to be dispatched.
self.create_builds(self.ppa)
build = self.builds[BuildStatus.SUPERSEDED.value]
build.notify()
- notification = pop_notifications()[1]
- self._assert_mail_is_correct(build, notification, ppa=True)
+ expected_reasons = [
+ ("test@xxxxxxxxxxx", "signer"),
+ (self.ppa_owner_email, "owner"),
+ ]
+ self._assert_mails_are_correct(build, expected_reasons, ppa=True)
def test_notify_currently_building(self):
- # We can notify the creator when the build is currently building.
+ # We can notify the creator and buildd admins when the build is
+ # currently building.
self.create_builds(self.archive)
build = self.builds[BuildStatus.BUILDING.value]
build.notify()
- notification = pop_notifications()[1]
- self._assert_mail_is_correct(build, notification)
+ expected_reasons = [
+ (email, "buildd-admin") for email in self.buildd_admins_email]
+ expected_reasons.append(("test@xxxxxxxxxxx", "creator"))
+ self._assert_mails_are_correct(build, expected_reasons)
def test_notify_currently_building_ppa(self):
- # We can notify the creator when the build is currently building.
+ # We can notify the signer and the archive owner when the build is
+ # currently building.
self.create_builds(self.ppa)
build = self.builds[BuildStatus.BUILDING.value]
build.notify()
- notification = pop_notifications()[1]
- self._assert_mail_is_correct(build, notification, ppa=True)
+ expected_reasons = [
+ ("test@xxxxxxxxxxx", "signer"),
+ (self.ppa_owner_email, "owner"),
+ ]
+ self._assert_mails_are_correct(build, expected_reasons, ppa=True)
def test_notify_uploading_build(self):
- # We can notify the creator when the build has completed, and binary
- # packages are being uploaded by the builder.
+ # We can notify the creator and buildd admins when the build has
+ # completed, and binary packages are being uploaded by the builder.
self.create_builds(self.archive)
build = self.builds[BuildStatus.UPLOADING.value]
build.notify()
- notification = pop_notifications()[1]
- self._assert_mail_is_correct(build, notification)
+ expected_reasons = [
+ (email, "buildd-admin") for email in self.buildd_admins_email]
+ expected_reasons.append(("test@xxxxxxxxxxx", "creator"))
+ self._assert_mails_are_correct(build, expected_reasons)
def test_notify_uploading_build_ppa(self):
- # We can notify the creator when the build has completed, and binary
- # packages are being uploaded by the builder.
+ # We can notify the signer and the archive owner when the build has
+ # completed, and binary packages are being uploaded by the builder.
self.create_builds(self.ppa)
build = self.builds[BuildStatus.UPLOADING.value]
build.notify()
- notification = pop_notifications()[1]
- self._assert_mail_is_correct(build, notification, ppa=True)
+ expected_reasons = [
+ ("test@xxxxxxxxxxx", "signer"),
+ (self.ppa_owner_email, "owner"),
+ ]
+ self._assert_mails_are_correct(build, expected_reasons, ppa=True)
def test_copied_into_ppa_does_not_spam(self):
# When a package is copied into a PPA, we don't send mail to the
@@ -304,10 +353,10 @@
self.distroseries, PackagePublishingPocket.RELEASE, self.ppa)
[ppa_build] = ppa_spph.createMissingBuilds()
ppa_build.notify()
- notifications = pop_notifications()
- self.assertEquals(1, len(notifications))
+ self._assert_mails_are_correct(
+ ppa_build, [(self.ppa_owner_email, "owner")], ppa=True)
- def test_notify_owner_supresses_mail(self):
+ def test_notify_owner_suppresses_mail(self):
# When the 'notify_owner' config option is False, we don't send mail
# to the owner of the SPR.
self.create_builds(self.archive)
@@ -319,13 +368,13 @@
""")
config.push('notify_owner', notify_owner)
build.notify()
- notifications = pop_notifications()
- actual_emails = [n['To'] for n in notifications]
- self.assertEquals(self.buildd_admins_email, actual_emails)
+ self._assert_mails_are_correct(
+ build,
+ [(email, "buildd-admin") for email in self.buildd_admins_email])
# And undo what we just did.
config.pop('notify_owner')
- def test_build_notification_supresses_mail(self):
+ def test_build_notification_suppresses_mail(self):
# When the 'build_notification' config option is False, we don't
# send any mail at all.
self.create_builds(self.archive)
@@ -337,12 +386,12 @@
config.push('send_build_notification', send_build_notification)
build.notify()
notifications = pop_notifications()
- self.assertEquals(0, len(notifications))
+ self.assertEqual(0, len(notifications))
# And undo what we just did.
config.pop('send_build_notification')
def test_sponsored_upload_notification(self):
- # If the signing key is different to the creator, they are both
+ # If the signing key is different from the creator, they are both
# notified.
sponsor = self.factory.makePerson('sponsor@xxxxxxxxxxx')
key = self.factory.makeGPGKey(owner=sponsor)
@@ -352,8 +401,8 @@
# Push past the security proxy
removeSecurityProxy(spr).dscsigningkey = key
build.notify()
- notifications = pop_notifications()
- expected_emails = self.buildd_admins_email + [
- 'sponsor@xxxxxxxxxxx', 'test@xxxxxxxxxxx']
- actual_emails = [n['To'] for n in notifications]
- self.assertEquals(expected_emails, actual_emails)
+ expected_reasons = [
+ (email, "buildd-admin") for email in self.buildd_admins_email]
+ expected_reasons.append(("test@xxxxxxxxxxx", "creator"))
+ expected_reasons.append(("sponsor@xxxxxxxxxxx", "signer"))
+ self._assert_mails_are_correct(build, expected_reasons)
Follow ups
