launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/silence-of-the-prober-ii into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #370329 Only probe Ubuntu mirrors?
  https://bugs.launchpad.net/bugs/370329
  #502842 CD image mirror prober failing
  https://bugs.launchpad.net/bugs/502842
  #721028 Mirror prober doesn't handle deactivated owners
  https://bugs.launchpad.net/bugs/721028

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/silence-of-the-prober-ii/+merge/50273

This branch fixes three issues in the mirror prober that make my inbox sad:

 - Sending a failure email for a mirror with a deactivated owner causes an exception due to a lack of To addresses. I've fixed this by first checking if the set of destination addresses is non-empty.
 - Some mirrors are too cool for 404s, instead redirecting to a special Not Found document. The prober logged these incidents as errors that required investigation. I've added them as an expected error alongside other invalid redirects and response codes, so they behave correctly like 404s.
 - Some non-Ubuntu mirrors exist, but we do not support probing them. This is a routine situation, not requiring any special attention. I have reduced its warning from INFO to DEBUG due to its unimportance.
-- 
https://code.launchpad.net/~wgrant/launchpad/silence-of-the-prober-ii/+merge/50273
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/silence-of-the-prober-ii into lp:launchpad.

=== modified file 'lib/lp/registry/model/distributionmirror.py'
--- lib/lp/registry/model/distributionmirror.py	2011-01-20 20:27:43 +0000
+++ lib/lp/registry/model/distributionmirror.py	2011-02-18 05:04:57 +0000
@@ -387,7 +387,8 @@
 
         if notify_owner:
             owner_address = get_contact_email_addresses(self.owner)
-            simple_sendmail(fromaddress, owner_address, subject, message)
+            if len(owner_address) > 0:
+                simple_sendmail(fromaddress, owner_address, subject, message)
 
     def newProbeRecord(self, log_file):
         """See IDistributionMirror"""

=== modified file 'lib/lp/registry/scripts/distributionmirror_prober.py'
--- lib/lp/registry/scripts/distributionmirror_prober.py	2010-12-24 17:08:10 +0000
+++ lib/lp/registry/scripts/distributionmirror_prober.py	2011-02-18 05:04:57 +0000
@@ -565,6 +565,7 @@
         BadResponseCode,
         ConnectionSkipped,
         ProberTimeout,
+        RedirectToDifferentFile,
         UnknownURLSchemeAfterRedirect,
         )
 
@@ -848,7 +849,7 @@
             # Some people registered mirrors on distros other than Ubuntu back
             # in the old times, so now we need to do this small hack here.
             if not mirror.distribution.full_functionality:
-                self.logger.info(
+                self.logger.debug(
                     "Mirror '%s' of distribution '%s' can't be probed --we "
                     "only probe Ubuntu mirrors."
                     % (mirror.name, mirror.distribution.name))

=== modified file 'lib/lp/registry/tests/test_distributionmirror.py'
--- lib/lp/registry/tests/test_distributionmirror.py	2011-01-20 17:54:24 +0000
+++ lib/lp/registry/tests/test_distributionmirror.py	2011-02-18 05:04:57 +0000
@@ -23,6 +23,7 @@
 from lp.registry.model.distributionmirror import DistributionMirror
 from lp.services.mail import stub
 from lp.services.worlddata.interfaces.country import ICountrySet
+from lp.testing import login_as
 from lp.testing.factory import LaunchpadObjectFactory
 
 
@@ -208,6 +209,25 @@
         self.failUnlessEqual(len(stub.test_emails), 0)
         stub.test_emails = []
 
+    def test_no_email_sent_to_uncontactable_owner(self):
+        # If the owner has no contact address, only the mirror admins are
+        # notified.
+        mirror = self.cdimage_mirror
+        login_as(mirror.owner)
+        # Deactivate the mirror owner to remove the contact address.
+        mirror.owner.deactivateAccount("I hate mirror spam.")
+        login_as(mirror.distribution.mirror_admin)
+        # Clear out notifications about the new team member.
+        transaction.commit()
+        stub.test_emails = []
+
+        # Disabling the mirror results in a single notification to the
+        # mirror admins.
+        self.factory.makeMirrorProbeRecord(mirror)
+        mirror.disable(notify_owner=True, log="It broke.")
+        transaction.commit()
+        self.failUnlessEqual(len(stub.test_emails), 1)
+
 
 class TestDistributionMirrorSet(unittest.TestCase):
     layer = LaunchpadFunctionalLayer

=== modified file 'lib/lp/registry/tests/test_distributionmirror_prober.py'
--- lib/lp/registry/tests/test_distributionmirror_prober.py	2010-12-24 10:03:15 +0000
+++ lib/lp/registry/tests/test_distributionmirror_prober.py	2011-02-18 05:04:57 +0000
@@ -57,6 +57,7 @@
     RedirectAwareProberFactory,
     RedirectAwareProberProtocol,
     RequestManager,
+    RedirectToDifferentFile,
     restore_http_proxy,
     should_skip_host,
     UnknownURLSchemeAfterRedirect,
@@ -666,11 +667,13 @@
                 BadResponseCode,
                 ProberTimeout,
                 ConnectionSkipped,
+                RedirectToDifferentFile,
                 UnknownURLSchemeAfterRedirect,
                 ]))
         exceptions = [BadResponseCode(str(httplib.NOT_FOUND)),
                       ProberTimeout('http://localhost/', 5),
                       ConnectionSkipped(),
+                      RedirectToDifferentFile('/foo', '/bar'),
                       UnknownURLSchemeAfterRedirect('https://localhost')]
         for exception in exceptions:
             failure = callbacks.ensureOrDeleteMirrorCDImageSeries(