launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #21236
[Merge] lp:~cjwatson/launchpad/better-upload-error-notifications into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/better-upload-error-notifications into lp:launchpad.
Commit message:
Send proper email notifications about most failures to parse the .changes file.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #499438 in Launchpad itself: "Better handling of fatal upload errors"
https://bugs.launchpad.net/launchpad/+bug/499438
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/better-upload-error-notifications/+merge/311179
We can now send email notifications to the signer as long as we have a signature, even if the signing key is deactivated. If the .changes file is sufficiently parseable then we'll notify other relevant people as well.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/better-upload-error-notifications into lp:launchpad.
=== modified file 'lib/lp/archiveuploader/changesfile.py'
--- lib/lp/archiveuploader/changesfile.py 2015-07-29 01:59:43 +0000
+++ lib/lp/archiveuploader/changesfile.py 2016-11-17 17:09:06 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
""" ChangesFile class
@@ -81,6 +81,11 @@
files = None
def __init__(self, filepath, policy, logger):
+ self.filepath = filepath
+ self.policy = policy
+ self.logger = logger
+
+ def parseChanges(self):
"""Process the given changesfile.
Does:
@@ -90,24 +95,25 @@
* Checks name of changes file
* Checks signature of changes file
- If any of these checks fail, UploadError is raised, and it's
- considered a fatal error (no subsequent processing of the upload
- will be done).
+ If any of these checks fail, UploadError is yielded, and it should
+ be considered a fatal error (no subsequent processing of the upload
+ should be done).
Logger and Policy are instances built in uploadprocessor.py passed
via NascentUpload class.
"""
- self.filepath = filepath
- self.policy = policy
- self.logger = logger
-
- self.parse(verify_signature=not policy.unsigned_changes_ok)
+ try:
+ self.parse(verify_signature=not self.policy.unsigned_changes_ok)
+ except UploadError as e:
+ yield e
+ return
for field in self.mandatory_fields:
if field not in self._dict:
- raise UploadError(
+ yield UploadError(
"Unable to find mandatory field '%s' in the changes "
"file." % field)
+ return
try:
format = float(self._dict["Format"])
@@ -116,7 +122,7 @@
format = 1.5
if format < 1.5 or format > 2.0:
- raise UploadError(
+ yield UploadError(
"Format out of acceptable range for changes file. Range "
"1.5 - 2.0, format %g" % format)
=== modified file 'lib/lp/archiveuploader/dscfile.py'
--- lib/lp/archiveuploader/dscfile.py 2016-06-01 01:59:32 +0000
+++ lib/lp/archiveuploader/dscfile.py 2016-11-17 17:09:06 +0000
@@ -144,6 +144,9 @@
if verify_signature:
self.signingkey, self.parsed_content = self.verifySignature(
self.raw_content, self.filepath)
+ if self.signingkey.active == False:
+ raise UploadError("File %s is signed with a deactivated key %s"
+ % (self.filepath, self.signingkey.keyid))
else:
self.logger.debug("%s can be unsigned." % self.filename)
self.parsed_content = self.raw_content
@@ -179,10 +182,6 @@
raise UploadError("Signing key %s not registered in launchpad."
% sig.fingerprint)
- if key.active == False:
- raise UploadError("File %s is signed with a deactivated key %s"
- % (filename, key.keyid))
-
return (key, sig.plain_data)
def parseAddress(self, addr, fieldname="Maintainer"):
=== modified file 'lib/lp/archiveuploader/nascentupload.py'
--- lib/lp/archiveuploader/nascentupload.py 2015-12-30 23:34:34 +0000
+++ lib/lp/archiveuploader/nascentupload.py 2016-11-17 17:09:06 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2014 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""The processing of nascent uploads.
@@ -141,6 +141,8 @@
policy = self.policy
self.logger.debug("Beginning processing.")
+ self.run_and_reject_on_error(self.changes.parseChanges)
+
try:
policy.setDistroSeriesAndPocket(self.changes.suite_name)
except NotFoundError:
@@ -774,7 +776,7 @@
IDistributionSet)['ubuntu'].currentseries
return distroseries.createQueueEntry(
PackagePublishingPocket.RELEASE,
- distroseries.main_archive, self.changes.filename,
+ self.policy.archive, self.changes.filename,
self.changes.raw_content, signing_key=self.changes.signingkey)
else:
return distroseries.createQueueEntry(
=== modified file 'lib/lp/archiveuploader/tests/nascentupload.txt'
--- lib/lp/archiveuploader/tests/nascentupload.txt 2016-01-26 15:47:37 +0000
+++ lib/lp/archiveuploader/tests/nascentupload.txt 2016-11-17 17:09:06 +0000
@@ -39,38 +39,34 @@
... name='anything', distro='ubuntu', distroseries='hoary')
-Constructing a NascentUpload object
------------------------------------
-
-Constructing a NascentUpload instance verifies that the changes file
-specified exists and tries to build a ChangesFile (see
-doc/nascentuploadfile.txt) object based on that. If anything goes
-wrong during this process UploadError is raised:
+NascentUpload Processing
+------------------------
+
+Processing a NascentUpload consists of building files objects for each
+specified file in the upload, executing all their specific checks and
+collect all errors that may be generated. (see doc/nascentuploadfile.txt)
+
+First, NascentUpload verifies that the changes file specified exist, and
+tries to build a ChangesFile (see doc/nascentuploadfile.txt) object based
+on that.
>>> from lp.services.log.logger import DevNullLogger, FakeLogger
- >>> NascentUpload.from_changesfile_path(
+
+ >>> nonexistent = NascentUpload.from_changesfile_path(
... datadir("DOES-NOT-EXIST"), buildd_policy, FakeLogger())
+ >>> nonexistent.process()
Traceback (most recent call last):
...
- UploadError:...
-
-Otherwise a ChangesFile object is ready to use.
+ EarlyReturnUploadError: An error occurred that prevented further
+ processing.
+ >>> nonexistent.is_rejected
+ True
+ >>> print nonexistent.rejection_message
+ Unable to read DOES-NOT-EXIST: ...
>>> quodlibet = NascentUpload.from_changesfile_path(
... datadir("quodlibet_0.13.1-1_i386.changes"),
... anything_policy, DevNullLogger())
-
- >>> quodlibet.changes
- <lp.archiveuploader.changesfile.ChangesFile ...>
-
-
-NascentUpload Processing
-------------------------
-
-Processing a NascentUpload consists of building files objects for each
-specified file in the upload, executing all their specific checks and
-collect all errors that may be generated. (see doc/nascentuploadfile.txt)
-
>>> quodlibet.process()
>>> for f in quodlibet.changes.files:
... print f.filename, f
=== modified file 'lib/lp/archiveuploader/tests/nascentuploadfile.txt'
--- lib/lp/archiveuploader/tests/nascentuploadfile.txt 2015-07-29 04:17:26 +0000
+++ lib/lp/archiveuploader/tests/nascentuploadfile.txt 2016-11-17 17:09:06 +0000
@@ -72,10 +72,14 @@
>>> ed_binary_changes = ChangesFile(
... datadir('ed_0.2-20_i386.changes.binary-only'),
... modified_insecure_policy, DevNullLogger())
+ >>> len(list(ed_binary_changes.parseChanges()))
+ 0
>>> ed_source_changes = ChangesFile(
... datadir('ed_0.2-20_source.changes'),
... modified_insecure_policy, DevNullLogger())
+ >>> len(list(ed_source_changes.parseChanges()))
+ 0
Make sure we are not getting any exceptions due to a malformed changes
file name.
=== modified file 'lib/lp/archiveuploader/tests/test_changesfile.py'
--- lib/lp/archiveuploader/tests/test_changesfile.py 2015-07-29 04:17:26 +0000
+++ lib/lp/archiveuploader/tests/test_changesfile.py 2016-11-17 17:09:06 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2011 Canonical Ltd. This software is licensed under the
+# Copyright 2010-2016 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Test ChangesFile functionality."""
@@ -175,7 +175,10 @@
changes.dump(changes_fd)
finally:
changes_fd.close()
- return ChangesFile(path, self.policy, self.logger)
+ changesfile = ChangesFile(path, self.policy, self.logger)
+ for error in changesfile.parseChanges():
+ raise error
+ return changesfile
def getBaseChanges(self):
contents = Changes()
@@ -357,36 +360,39 @@
import_public_test_keys()
def test_valid_signature_accepted(self):
- # A correctly signed changes file is excepted, and all its
+ # A correctly signed changes file is accepted, and all its
# content is parsed.
path = datadir('signatures/signed.changes')
- parsed = ChangesFile(path, InsecureUploadPolicy(), BufferLogger())
+ changesfile = ChangesFile(path, InsecureUploadPolicy(), BufferLogger())
+ self.assertEqual([], list(changesfile.parseChanges()))
self.assertEqual(
getUtility(IPersonSet).getByEmail('foo.bar@xxxxxxxxxxxxx'),
- parsed.signer)
+ changesfile.signer)
expected = "\AFormat: 1.7\n.*foo_1.0-1.diff.gz\Z"
self.assertTextMatchesExpressionIgnoreWhitespace(
expected,
- parsed.parsed_content)
+ changesfile.parsed_content)
def test_no_signature_rejected(self):
# An unsigned changes file is rejected.
path = datadir('signatures/unsigned.changes')
- self.assertRaises(
- UploadError,
- ChangesFile, path, InsecureUploadPolicy(), BufferLogger())
+ changesfile = ChangesFile(path, InsecureUploadPolicy(), BufferLogger())
+ errors = list(changesfile.parseChanges())
+ self.assertIsInstance(errors[0], UploadError)
+ self.assertEqual(1, len(errors))
def test_prefix_ignored(self):
# A signed changes file with an unsigned prefix has only the
# signed part parsed.
path = datadir('signatures/prefixed.changes')
- parsed = ChangesFile(path, InsecureUploadPolicy(), BufferLogger())
+ changesfile = ChangesFile(path, InsecureUploadPolicy(), BufferLogger())
+ self.assertEqual([], list(changesfile.parseChanges()))
self.assertEqual(
getUtility(IPersonSet).getByEmail('foo.bar@xxxxxxxxxxxxx'),
- parsed.signer)
+ changesfile.signer)
expected = "\AFormat: 1.7\n.*foo_1.0-1.diff.gz\Z"
self.assertTextMatchesExpressionIgnoreWhitespace(
expected,
- parsed.parsed_content)
- self.assertEqual("breezy", parsed.suite_name)
- self.assertNotIn("evil", parsed.changes_comment)
+ changesfile.parsed_content)
+ self.assertEqual("breezy", changesfile.suite_name)
+ self.assertNotIn("evil", changesfile.changes_comment)
=== modified file 'lib/lp/archiveuploader/tests/test_nascentuploadfile.py'
--- lib/lp/archiveuploader/tests/test_nascentuploadfile.py 2016-11-08 20:22:59 +0000
+++ lib/lp/archiveuploader/tests/test_nascentuploadfile.py 2016-11-17 17:09:06 +0000
@@ -208,7 +208,9 @@
path = os.path.join(tempdir, filename)
with open(path, "w") as changes_fd:
changes.dump(changes_fd)
- return ChangesFile(path, self.policy, self.logger)
+ changesfile = ChangesFile(path, self.policy, self.logger)
+ self.assertEqual([], list(changesfile.parseChanges()))
+ return changesfile
class DSCFileTests(PackageUploadFileTestCase):
=== modified file 'lib/lp/archiveuploader/tests/test_sync_notification.py'
--- lib/lp/archiveuploader/tests/test_sync_notification.py 2015-08-26 13:41:21 +0000
+++ lib/lp/archiveuploader/tests/test_sync_notification.py 2016-11-17 17:09:06 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012-2015 Canonical Ltd. This software is licensed under the
+# Copyright 2012-2016 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."""
@@ -55,6 +55,7 @@
self.raw_content = open(file_path).read()
self.signingkey = None
+ parseChanges = FakeMethod([])
checkFileName = FakeMethod([])
processAddresses = FakeMethod([])
processFiles = FakeMethod([])
=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
--- lib/lp/archiveuploader/tests/test_uploadprocessor.py 2016-07-02 07:55:26 +0000
+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py 2016-11-17 17:09:06 +0000
@@ -51,6 +51,7 @@
IBuildFarmJobBehaviour,
)
from lp.registry.interfaces.distribution import IDistributionSet
+from lp.registry.interfaces.gpg import IGPGKeySet
from lp.registry.interfaces.person import IPersonSet
from lp.registry.interfaces.pocket import PackagePublishingPocket
from lp.registry.interfaces.series import SeriesStatus
@@ -239,7 +240,7 @@
excName = excClass.__name__
else:
excName = str(excClass)
- raise self.failureException, "%s not raised" % excName
+ raise self.failureException("%s not raised" % excName)
def setupBreezy(self, name="breezy", permitted_formats=None):
"""Create a fresh distroseries in ubuntu.
@@ -1954,10 +1955,47 @@
self.assertEqual(UploadStatusEnum.REJECTED, result)
self.assertLogContains(
- "INFO Failed to parse changes file")
+ "INFO Not sending rejection notice without a signing key.")
self.assertEmailQueueLength(0)
self.assertEqual([], self.oopses)
+ def test_deactivated_key_upload_sends_mail(self):
+ # An upload signed with a deactivated key does not OOPS and sends a
+ # rejection email.
+ self.switchToAdmin()
+ fingerprint = "340CA3BB270E2716C9EE0B768E7EB7086C64A8C5"
+ gpgkeyset = getUtility(IGPGKeySet)
+ gpgkeyset.deactivate(gpgkeyset.getByFingerprint(fingerprint))
+ self.switchToUploader()
+
+ uploadprocessor = self.setupBreezyAndGetUploadProcessor()
+ upload_dir = self.queueUpload("netapplet_1.0-1-signed")
+
+ [result] = self.processUpload(uploadprocessor, upload_dir)
+
+ self.assertEqual(UploadStatusEnum.REJECTED, result)
+ base_contents = [
+ "Subject: [ubuntu] netapplet_1.0-1_source.changes (Rejected)",
+ "File %s/netapplet_1.0-1-signed/netapplet_1.0-1_source.changes "
+ "is signed with a deactivated key %s" % (
+ self.incoming_folder, fingerprint[-8:]),
+ ]
+ expected = []
+ expected.append({
+ "contents": base_contents + [
+ "You are receiving this email because you are the most "
+ "recent person",
+ "listed in this package's changelog."],
+ "recipient": "daniel.silverstone@xxxxxxxxxxxxx",
+ })
+ expected.append({
+ "contents": base_contents + [
+ "You are receiving this email because you made this upload."],
+ "recipient": "foo.bar@xxxxxxxxxxxxx",
+ })
+ self.assertEmails(expected)
+ self.assertEqual([], self.oopses)
+
def test_ddeb_upload_overrides(self):
# DDEBs should always be overridden to the same values as their
# counterpart DEB's.
=== modified file 'lib/lp/archiveuploader/uploadprocessor.py'
--- lib/lp/archiveuploader/uploadprocessor.py 2015-08-03 15:07:29 +0000
+++ lib/lp/archiveuploader/uploadprocessor.py 2016-11-17 17:09:06 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Code for 'processing' 'uploads'. Also see nascentupload.py.
@@ -338,22 +338,8 @@
# The path we want for NascentUpload is the path to the folder
# containing the changes file (and the other files referenced by it).
changesfile_path = os.path.join(self.upload_path, changes_file)
- try:
- upload = NascentUpload.from_changesfile_path(
- changesfile_path, policy, self.processor.log)
- except UploadError as e:
- # We failed to parse the changes file, so we have no key or
- # Changed-By to notify of the rejection. Just log it and
- # move on.
- # XXX wgrant 2011-09-29 bug=499438: With some refactoring we
- # could do better here: if we have a signature then we have
- # somebody to email, even if the rest of the file is
- # corrupt.
- logger.info(
- "Failed to parse changes file '%s': %s" % (
- os.path.join(self.upload_path, changes_file),
- str(e)))
- return UploadStatusEnum.REJECTED
+ upload = NascentUpload.from_changesfile_path(
+ changesfile_path, policy, self.processor.log)
# Reject source upload to buildd upload paths.
first_path = relative_path.split(os.path.sep)[0]
@@ -411,7 +397,16 @@
notify = False
if upload.is_rejected:
result = UploadStatusEnum.REJECTED
- upload.do_reject(notify)
+ if upload.changes.parsed_content is not None:
+ # We got past the point of checking any required
+ # signature, so we can do a proper rejection.
+ upload.do_reject(notify)
+ else:
+ # The upload required a signature and either didn't have
+ # one or we failed to verify it, so we have nobody to
+ # notify. Just log it and move on.
+ logger.info(
+ "Not sending rejection notice without a signing key.")
self.processor.ztm.abort()
else:
successful = self._acceptUpload(upload, notify)
=== modified file 'lib/lp/soyuz/doc/distroseriesqueue-translations.txt'
--- lib/lp/soyuz/doc/distroseriesqueue-translations.txt 2015-09-04 12:19:07 +0000
+++ lib/lp/soyuz/doc/distroseriesqueue-translations.txt 2016-11-17 17:09:06 +0000
@@ -87,10 +87,9 @@
>>> pmount_upload = NascentUpload.from_changesfile_path(
... datadir('pmount_0.9.7-2ubuntu2_amd64.changes'),
... buildd_policy, FakeLogger())
- DEBUG pmount_0.9.7-2ubuntu2_amd64.changes can be unsigned.
-
>>> pmount_upload.process(build=build)
DEBUG Beginning processing.
+ DEBUG pmount_0.9.7-2ubuntu2_amd64.changes can be unsigned.
DEBUG Verifying the changes file.
DEBUG Verifying files in upload.
DEBUG Verifying binary pmount_0.9.7-2ubuntu2_amd64.deb
Follow ups
