launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06039
[Merge] lp:~jtv/launchpad/bug-876594 into lp:launchpad
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-876594 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #876594 in Launchpad itself: "rejected builds for synced packages send mail to Debian maintainer"
https://bugs.launchpad.net/launchpad/+bug/876594
For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-876594/+merge/87624
= Summary =
We're spamming Debian maintainers with failure notifications from builds of their packages as copied to Ubuntu (or derived distros).
== Proposed fix ==
Spam the person who requested the sync instead.
== Pre-implementation notes ==
Discussed with Colin, Julian, and William. There may be opportunities to simplify other notification logic in the future, in particular where it suppresses inappropriate notifications for PPAs.
Apparently a build upload has no signing key; we're not entirely sure right now whose signing key is on the upload. We kept the existing logic for non-copied uploads.
== Implementation details ==
See the bug for call trees.
I didn't want to mess with the notify() function underlying Upload.notify(); it's got weird special cases and is used in too many code paths and data flows. It seems to be picking the wrong default victim in cases where there is no signing key.
== Tests ==
I added a new one. Giving it a file of itself allowed me to introduce lots of helpers.
{{{
./bin/test -vvc lp.archiveuploader.tests.test_sync_notification
}}}
== Demo and Q/A ==
We'll need help from the Soyuz specialists for this.
= Launchpad lint =
There's one piece of pre-existing lint that I dared not touch. Something about a getter and setter with the same name confusing the linter, I think.
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/archiveuploader/nascentupload.py
lib/lp/soyuz/model/queue.py
lib/lp/archiveuploader/tests/test_sync_notification.py
lib/lp/soyuz/model/archive.py
./lib/lp/soyuz/model/archive.py
348: redefinition of function 'private' from line 344
--
https://code.launchpad.net/~jtv/launchpad/bug-876594/+merge/87624
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-876594 into lp:launchpad.
=== modified file 'lib/lp/archiveuploader/nascentupload.py'
--- lib/lp/archiveuploader/nascentupload.py 2011-12-24 13:09:35 +0000
+++ lib/lp/archiveuploader/nascentupload.py 2012-01-05 14:19:44 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""The processing of nascent uploads.
@@ -909,10 +909,10 @@
# state.
pass
- changes_file_object = open(self.changes.filepath, "r")
- self.queue_root.notify(summary_text=self.rejection_message,
- changes_file_object=changes_file_object, logger=self.logger)
- changes_file_object.close()
+ with open(self.changes.filepath, "r") as changes_file_object:
+ self.queue_root.notify(
+ summary_text=self.rejection_message,
+ changes_file_object=changes_file_object, logger=self.logger)
def _createQueueEntry(self):
"""Return a PackageUpload object."""
=== added file 'lib/lp/archiveuploader/tests/test_sync_notification.py'
--- lib/lp/archiveuploader/tests/test_sync_notification.py 1970-01-01 00:00:00 +0000
+++ lib/lp/archiveuploader/tests/test_sync_notification.py 2012-01-05 14:19:44 +0000
@@ -0,0 +1,158 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test notification behaviour for cross-distro package syncs."""
+
+__metaclass__ = type
+
+import os.path
+
+from zope.component import getUtility
+
+from lp.archiveuploader.nascentupload import (
+ NascentUpload,
+ UploadError,
+ )
+from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.services.log.logger import DevNullLogger
+from lp.soyuz.enums import (
+ ArchivePermissionType,
+ SourcePackageFormat,
+ )
+from lp.soyuz.interfaces.sourcepackageformat import (
+ ISourcePackageFormatSelectionSet,
+ )
+from lp.soyuz.model.archivepermission import ArchivePermission
+from lp.soyuz.scripts.packagecopier import do_copy
+from lp.testing import (
+ login,
+ TestCaseWithFactory,
+ )
+from lp.testing.fakemethod import FakeMethod
+from lp.testing.layers import LaunchpadZopelessLayer
+from lp.testing.mail_helpers import pop_notifications
+
+
+class FakeUploadPolicy:
+ def __init__(self, spph):
+ self.distroseries = spph.distroseries
+ self.archive = spph.distroseries.main_archive
+ self.pocket = spph.pocket
+
+ setDistroSeriesAndPocket = FakeMethod()
+ validateUploadType = FakeMethod()
+ checkUpload = FakeMethod()
+
+
+class FakeChangesFile:
+ def __init__(self, spph, file_path, maintainer_key):
+ self.files = []
+ self.filepath = file_path
+ self.filename = os.path.basename(file_path)
+ self.architectures = ['i386']
+ self.suite_name = '-'.join([spph.distroseries.name, spph.pocket.name])
+ self.raw_content = open(file_path).read()
+ self.signingkey = maintainer_key
+
+ checkFileName = FakeMethod([])
+ processAddresses = FakeMethod([])
+ processFiles = FakeMethod([])
+ verify = FakeMethod([UploadError("Deliberately broken")])
+
+
+class TestSyncNotification(TestCaseWithFactory):
+
+ layer = LaunchpadZopelessLayer
+
+ def makePersonWithEmail(self):
+ """Create a person; return (person, email)."""
+ address = "%s@xxxxxxxxxxx" % self.factory.getUniqueString()
+ person = self.factory.makePerson(email=address)
+ return person, address
+
+ def makeSPPH(self, distroseries, maintainer_address):
+ """Create a `SourcePackagePublishingHistory`."""
+ return self.factory.makeSourcePackagePublishingHistory(
+ distroseries=distroseries, pocket=PackagePublishingPocket.RELEASE,
+ dsc_maintainer_rfc822=maintainer_address)
+
+ def makeUploader(self, person, archive, component):
+ """Grant a person upload privileges for archive/component."""
+ ArchivePermission(
+ person=person, archive=archive, component=component,
+ permission=ArchivePermissionType.UPLOAD)
+
+ def syncSource(self, spph, target_distroseries, requester):
+ """Sync `spph` into `target_distroseries`."""
+ getUtility(ISourcePackageFormatSelectionSet).add(
+ target_distroseries, SourcePackageFormat.FORMAT_1_0)
+ target_archive = target_distroseries.main_archive
+ self.makeUploader(requester, target_archive, spph.component)
+ [synced_spph] = do_copy(
+ [spph], target_archive, target_distroseries,
+ pocket=spph.pocket, person=requester, allow_delayed_copies=False,
+ close_bugs=False)
+ return synced_spph
+
+ def makeChangesFile(self, spph, maintainer, maintainer_address):
+ maintainer_key = self.factory.makeGPGKey(maintainer)
+ temp_dir = self.makeTemporaryDirectory()
+ changes_file = os.path.join(
+ temp_dir, "%s.changes" % spph.source_package_name)
+ open(changes_file, 'w').write(
+ "Maintainer: %s <%s>\n" % (maintainer.name, maintainer_address))
+ return FakeChangesFile(spph, changes_file, maintainer_key)
+
+ def makeNascentUpload(self, spph, maintainer, maintainer_address):
+ """Create a `NascentUpload` for `spph`."""
+ upload = NascentUpload(
+ self.makeChangesFile(spph, maintainer, maintainer_address),
+ FakeUploadPolicy(spph), DevNullLogger())
+ upload.queue_root = upload._createQueueEntry()
+ das = self.factory.makeDistroArchSeries(
+ distroseries=spph.distroseries)
+ bpb = self.factory.makeBinaryPackageBuild(
+ source_package_release=spph.sourcepackagerelease,
+ archive=spph.archive, distroarchseries=das, pocket=spph.pocket,
+ sourcepackagename=spph.sourcepackagename)
+ upload.queue_root.addBuild(bpb)
+ return upload
+
+ def processAndRejectUpload(self, nascent_upload):
+ nascent_upload.process()
+ # Obtain the required privileges for do_reject.
+ login('foo.bar@xxxxxxxxxxxxx')
+ nascent_upload.do_reject(notify=True)
+
+ def getNotifiedAddresses(self):
+ """Get email addresses that were notified."""
+ return [message['to'] for message in pop_notifications()]
+
+ def test_maintainer_not_notified_about_build_failure_elsewhere(self):
+ """No mail to maintainers about builds they're not responsible for.
+
+
+ We import Debian source packages, then sync them into Ubuntu (and
+ from there, into Ubuntu-derived distros). Those syncs then trigger
+ builds that the original Debian maintainers are not responsible for.
+
+ In a situation like that, we should not bother the maintainer with
+ the failure. We notify the person who requested the sync instead.
+
+ This test guards against bug 876594.
+ """
+ maintainer, maintainer_address = self.makePersonWithEmail()
+ dsp = self.factory.makeDistroSeriesParent()
+ original_spph = self.makeSPPH(dsp.parent_series, maintainer_address)
+ sync_requester, syncer_address = self.makePersonWithEmail()
+ synced_spph = self.syncSource(
+ original_spph, dsp.derived_series, sync_requester)
+ nascent_upload = self.makeNascentUpload(
+ synced_spph, maintainer, maintainer_address)
+ pop_notifications()
+ self.processAndRejectUpload(nascent_upload)
+
+ notified_addresses = '\n'.join(self.getNotifiedAddresses())
+
+ self.assertNotIn(maintainer_address, notified_addresses)
+ self.assertIn(syncer_address, notified_addresses)
=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py 2011-12-30 06:14:56 +0000
+++ lib/lp/soyuz/model/archive.py 2012-01-05 14:19:44 +0000
@@ -1288,18 +1288,14 @@
# - the given source package directly
# - a package set in the correct distro series that includes the
# given source package
- source_allowed = self.checkArchivePermission(person,
- sourcepackagename)
+ source_allowed = self.checkArchivePermission(
+ person, sourcepackagename)
set_allowed = self.isSourceUploadAllowed(
sourcepackagename, person, distroseries)
if source_allowed or set_allowed:
return None
if not self.getComponentsForUploader(person):
- # XXX: JamesWestby 2010-08-01 bug=612351: We have to use
- # is_empty() as we don't get an SQLObjectResultSet back, and
- # so __nonzero__ isn't defined on it, and a straight bool
- # check wouldn't do the right thing.
if self.getPackagesetsForUploader(person).is_empty():
return NoRightsForArchive()
else:
=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py 2011-12-30 06:14:56 +0000
+++ lib/lp/soyuz/model/queue.py 2012-01-05 14:19:44 +0000
@@ -845,6 +845,27 @@
# and uploading to any archive as the signer.
return changes, strip_pgp_signature(changes_content).splitlines(True)
+ def findSourcePublication(self):
+ """Find the `SourcePackagePublishingHistory` for this build."""
+ first_build = self.builds[:1]
+ if first_build:
+ [first_build] = first_build
+ return first_build.build._getLatestPublication()
+ else:
+ return None
+
+ def findPersonToNotify(self):
+ """Find the right person to notify about this upload."""
+ spph = self.findSourcePublication()
+ if spph and self.sourcepackagerelease.upload_archive != self.archive:
+ # This is a build triggered by the syncing of a source
+ # package. Notify the person who requested the sync.
+ return spph.creator
+ elif self.signing_key:
+ return self.signing_key.owner
+ else:
+ return None
+
def notify(self, summary_text=None, changes_file_object=None,
logger=None, dry_run=False):
"""See `IPackageUpload`."""
@@ -860,12 +881,9 @@
changesfile_content = changes_file_object.read()
else:
changesfile_content = 'No changes file content available.'
- if self.signing_key is not None:
- signer = self.signing_key.owner
- else:
- signer = None
+ blamee = self.findPersonToNotify()
notify(
- signer, self.sourcepackagerelease, self.builds, self.customfiles,
+ blamee, self.sourcepackagerelease, self.builds, self.customfiles,
self.archive, self.distroseries, self.pocket, summary_text,
changes, changesfile_content, changes_file_object,
status_action[self.status], dry_run=dry_run, logger=logger)
