Launchpad itself

Merge lp://staging/~gary/launchpad/bug650343 into lp://staging/launchpad

bug650343
Merge into devel

Proposed by Gary Poster on 2010-10-05

Status:

Merged

Approved by:

Māris Fogels on 2010-10-05

Approved revision:

no longer in the source branch.

Merged at revision:

11705

Proposed branch:

lp://staging/~gary/launchpad/bug650343

Merge into:

lp://staging/launchpad

Diff against target:

313 lines (+85/-48)

3 files modified

lib/canonical/launchpad/doc/incomingmail.txt (+65/-32)
lib/canonical/launchpad/mail/incoming.py (+19/-15)
lib/lp/blueprints/doc/spec-mail-exploder.txt (+1/-1)

To merge this branch:

bzr merge lp://staging/~gary/launchpad/bug650343

High

Fix Released

Link a bug report

Reviewer	Date Requested	Status
Francis J. Lacoste (community)		Approve on 2010-10-12
Māris Fogels (community)	2010-10-05	Approve on 2010-10-05
Review via email: mp+37633@code.staging.launchpad.net

Description of the change

See bug 650343 for the motivation. This is primarily just reverting the change done described in that bug, because the constraint/problem in the data center email configuration has been resolved.

I kept a spelling correction from the change. I also changed things per what lint described (e.g., converted to ReST), except for the singleton tuple complaints (e.g., in "log.warning('DKIM error: %r' % (e,))" the linter wants to see "(e, )").

./lib/canonical/launchpad/mail/incoming.py
     126: E231 missing whitespace after ','
     130: E231 missing whitespace after ','
     133: E231 missing whitespace after ','
     134: E231 missing whitespace after ','
     142: E231 missing whitespace after ','
     153: E231 missing whitespace after ','

Revision history for this message

Gary Poster (gary) wrote on 2010-10-05:

Sorry, bug 650343.

Revision history for this message

Māris Fogels (mars) wrote on 2010-10-05:

Hi Gary,

The code for this change looks good to land. r=mars. I have a few questions about the tests though:

* Do we want to silence the log warnings in the doctest output on line 26 of the diff? They look really odd. If we can not silence them, we could at least explain why they were left in there, and why there are four of them.

* We need some way to move tests like this into lib/lp. The problem is that this test lives in canonical/launchpad/doc, which runs in a different layer than the pagetests (LaunchpadFunctionalLayer I believe), and I do not think we have a place yet in the lib/lp that runs tests at this level.

This looks good to me. r=mars.

Maris

review: Approve

Revision history for this message

Jonathan Lange (jml) wrote on 2010-10-05:

On Tue, Oct 5, 2010 at 5:37 PM, Māris Fogels <email address hidden> wrote:
> * We need some way to move tests like this into lib/lp. The problem is that this test lives in canonical/launchpad/doc, which runs in a different layer than the pagetests (LaunchpadFunctionalLayer I believe), and I do not think we have a place yet in the lib/lp that runs tests at this level.

It's easy enough to move them. Just copy what other lib/lp packages do
for their doctests.

jml

Revision history for this message

Francis J. Lacoste (flacoste) wrote on 2010-10-05:

The header name should be changed to X-Launchpad-Original-To.

review: Needs Fixing

Revision history for this message

Gary Poster (gary) wrote on 2010-10-11:

This branch now has the change Francis pointed out was necessary, and so is ready for his review.

The requests for moving files around from jml and mars resulted in a 600+ line diff (`make lint` always gets its chunk of flesh) that can be found in its own branch: https://code.edge.launchpad.net/~gary/launchpad/bug650343-2 . That is running through ec2 and I'll ask for a review when I get a successful test run.