Better handling of fatal upload errors
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
Launchpad itself |
Fix Released
|
High
|
Colin Watson |
Bug Description
Currently when a person uploads a bad source, such as a changes file without a mandatory binary field, a FatalUploadError is raised, and an oops created but the user never receives an email response.
Examples:
http://
After uploading the package to dogfood, we were able to see the (obvious) issue:
http://
FatalUploadError: Unable to find mandatory field 'Binary' in the changes file.
OOPS-1921BUILDMASTER1, OOPS-1921BUILDMASTER2, OOPS-1921PPA6
FatalUploadError: Unable to find mandatory field 'Files' in the changes file.
OOPS-1921PPA4, OOPS-1921PPA5
FatalUploadError: Unable to find mandatory field 'Date' in the changes file.
OOPS-2021FTPMASTER5
FatalUploadError: File /srv/launchpad.
OOPS-2035PPA5
FatalUploadError: Signing key 0D9EDC5F633B05D
OOPS-2047PPA10
We should handle this exception and notify the user via email.
Related branches
- William Grant: Approve (code)
-
Diff: 438 lines (+129/-76)11 files modifiedlib/lp/archiveuploader/changesfile.py (+16/-10)
lib/lp/archiveuploader/dscfile.py (+10/-6)
lib/lp/archiveuploader/nascentupload.py (+3/-1)
lib/lp/archiveuploader/tests/nascentupload.txt (+19/-23)
lib/lp/archiveuploader/tests/nascentuploadfile.txt (+4/-0)
lib/lp/archiveuploader/tests/test_changesfile.py (+19/-13)
lib/lp/archiveuploader/tests/test_nascentuploadfile.py (+3/-1)
lib/lp/archiveuploader/tests/test_sync_notification.py (+2/-1)
lib/lp/archiveuploader/tests/test_uploadprocessor.py (+40/-2)
lib/lp/archiveuploader/uploadprocessor.py (+12/-17)
lib/lp/soyuz/doc/distroseriesqueue-translations.txt (+1/-2)
tags: | added: soyuz-upload |
Changed in soyuz: | |
milestone: | none → 10.01 |
status: | New → Triaged |
importance: | Undecided → High |
Changed in soyuz: | |
assignee: | nobody → Michael Nelson (michael.nelson) |
Changed in soyuz: | |
status: | Triaged → In Progress |
Changed in soyuz: | |
status: | In Progress → Triaged |
assignee: | Michael Nelson (michael.nelson) → nobody |
Changed in soyuz: | |
milestone: | 10.01 → 10.02 |
importance: | High → Low |
description: | updated |
Changed in soyuz: | |
milestone: | 10.02 → none |
description: | updated |
description: | updated |
tags: | added: oops |
Changed in launchpad: | |
importance: | Low → Critical |
description: | updated |
description: | updated |
Changed in launchpad: | |
assignee: | nobody → Julian Edwards (julian-edwards) |
description: | updated |
description: | updated |
Changed in launchpad: | |
assignee: | nobody → Colin Watson (cjwatson) |
status: | Triaged → In Progress |
information type: | Public → Private |
information type: | Private → Public |
tags: |
added: qa-ok removed: qa-needstesting |
Changed in launchpad: | |
status: | Fix Committed → Fix Released |
Pre-implementation notes requiring feedback:
After a brief survey of the code in lp.archiveuploa der.changesfile .ChangesFile. __init_ _(), it seems that we generate an oops (rather than rejecting the upload with a nice email) if:
1. The changes file can't be parsed correctly,
2. *any* of the mandatory fields are not present (currently source, binary, architecture, version, distribution, maintainer, files, changes),
3. the format of the changes file is not supported, or
4. the signature could not be verified (for a number of reasons).
I assume (2) is happening because if we can't verify the maintainer (one of the mandatory fields), we know we can't email them, but I'm not sure why we've included the other fields in this check.
So, the plan is to update (2) above simply to:
2. The maintainer field is not present (hrm, should really be changed-by? What's the reason that changed-by is not already a mandatory field in this check?)
and remove (3) altogether from __init__, and then add two new methods:
ChangesFile. checkMandatoryF ields() checkChangesFor mat()
ChangesFile.
which can be used by NascentUpload. process (which calls run_and_ reject_ on_errors) .
The result will be that we only oops in situations where we don't have a signer or maintainer email.
Note: at first I was confused why we don't include changed-by as a mandatory field, but realised it might be because it's not required by policy (http:// www.debian. org/doc/ debian- policy/ ch-controlfield s.html# s-debianchanges files). But we do raise an UploadError during checkAddresses later if it's not present (which doesn't generate an oops. but rejects the upload, which will try to email the signer as per normal).
Note: currently PackageUpload. _getRecipients( ) always includes either the signer or the changed-by address. But even though these may be present, an email will still not be sent if they have no preferredEmail etc.