Better handling of fatal upload errors
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
| Launchpad itself |
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) on 2017-07-24
-
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 |
Michael Nelson (michael.nelson) wrote : | #1 |
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 |
William Grant (wgrant) wrote : | #2 |
The OOPS is being handled separately, in bug #862032. This bug is now about trying harder to notify; in some cases we have a valid signature, so we can notify the signer even if the rest of the file is corrupt.
Changed in launchpad: | |
importance: | Critical → High |
tags: | removed: oops |
description: | updated |
Robert Collins (lifeless) wrote : | #3 |
Note that this would be much easier if we knew the uploader and thus could just notify their account. We do for SFTP uploads. I'm inclined to suggest that we deprecate FTP uploads and spend no effort to make them better - diminishing returns, and a known complex stack which is hard for new users (exactly the users that will make bad packages) to work with,
Robert Collins (lifeless) wrote : | #4 |
Also, @bigjools, are you hacking on this still?
Julian Edwards (julian-edwards) wrote : | #5 |
Nope, removed myself.
Changed in launchpad: | |
assignee: | Julian Edwards (julian-edwards) → nobody |
Julian Edwards (julian-edwards) wrote : | #6 |
"I'm inclined to suggest that we deprecate FTP uploads"
That will not happen - you will encounter *serious* resistance from Ubuntu people.
Francis J. Lacoste (flacoste) wrote : | #7 |
@julian resistance on what ground?
Julian Edwards (julian-edwards) wrote : | #8 |
* dput with SFTP provides no progress information
* client support in Ubuntu is recent and the dput.cf defaults to FTP so it's not reasonable to disable it in LP until that's changed, including stable releases
* the dput.cf for SFTP has to have the user's LP name in it, so is hard to have a default at all
* Debian's dput doesn't have SFTP support yet
It's not insurmountable but has both political and technical issues.
On Thu, Jan 5, 2012 at 5:45 AM, Julian Edwards
<email address hidden> wrote:
> * dput with SFTP provides no progress information
> * client support in Ubuntu is recent and the dput.cf defaults to FTP so it's not reasonable to disable it in LP until that's changed, including stable releases
> * the dput.cf for SFTP has to have the user's LP name in it, so is hard to have a default at all
> * Debian's dput doesn't have SFTP support yet
>
> It's not insurmountable but has both political and technical issues.
FWIW I talked with cjwatson about this before adding that comment.
Dput could default to sftp, prompt for the username, and fallback to
FTP if none was supplied or there was no terminal on which to prompt.
If it succeeded with a username, it would save it in a personal
dotfile and use it the next time.
-Rob
Changed in launchpad: | |
assignee: | nobody → Colin Watson (cjwatson) |
status: | Triaged → In Progress |
information type: | Public → Private |
information type: | Private → Public |
Launchpad QA Bot (lpqabot) wrote : | #10 |
Fixed in stable r18472 <http://
tags: | added: qa-needstesting |
Changed in launchpad: | |
status: | In Progress → Fix Committed |
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.