launchpad-reviewers team mailing list archive

[Celso Providelo <celso.providelo@xxxxxxxxxxxxx>]

Celso Providelo has proposed merging lp:~cprov/launchpad/pubdistro-bug-1290481 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1290481 in Launchpad itself: "Gap between publishing custom uploads and signing them"
  https://bugs.launchpad.net/launchpad/+bug/1290481

For more details, see:
https://code.launchpad.net/~cprov/launchpad/pubdistro-bug-1290481/+merge/210335
-- 
https://code.launchpad.net/~cprov/launchpad/pubdistro-bug-1290481/+merge/210335
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cprov/launchpad/pubdistro-bug-1290481 into lp:launchpad.

=== modified file 'lib/lp/archivepublisher/scripts/publish_ftpmaster.py'
--- lib/lp/archivepublisher/scripts/publish_ftpmaster.py	2012-06-22 17:26:53 +0000
+++ lib/lp/archivepublisher/scripts/publish_ftpmaster.py	2014-03-11 03:51:26 +0000
@@ -532,16 +532,20 @@
         self.runParts(distribution, 'finalize.d', env)
 
     def publishSecurityUploads(self, distribution):
-        """Quickly process just the pending security uploads."""
+        """Quickly process just the pending security uploads.
+
+        Returns True if publications were made, False otherwise.
+        """
         self.logger.debug("Expediting security uploads.")
         security_suites = self.getDirtySecuritySuites(distribution)
         if len(security_suites) == 0:
             self.logger.debug("Nothing to do for security publisher.")
-            return
+            return False
 
         self.publishDistroArchive(
             distribution, distribution.main_archive,
             security_suites=security_suites)
+        return True
 
     def publishDistroUploads(self, distribution):
         """Publish the distro's complete uploads."""
@@ -559,12 +563,17 @@
             updates on the main archive.  This is much faster, so it
             makes sense to do a security-only run before the main
             event to expedite critical fixes.
+        :return has_published: True if any publication was made to the
+            archive.
         """
+        has_published = False
         try:
             if security_only:
-                self.publishSecurityUploads(distribution)
+                has_published = self.publishSecurityUploads(distribution)
             else:
                 self.publishDistroUploads(distribution)
+                # Let's assume the main archive is always modified
+                has_published = True
 
             # Swizzle the now-updated backup dists and the current dists
             # around.
@@ -578,6 +587,8 @@
             self.recoverWorkingDists()
             raise
 
+        return has_published
+
     def prepareFreshSeries(self, distribution):
         """If there are any new distroseries, prepare them for publishing.
 
@@ -614,8 +625,8 @@
         self.processAccepted(distribution)
 
         self.rsyncBackupDists(distribution)
-        self.publish(distribution, security_only=True)
-        self.runFinalizeParts(distribution, security_only=True)
+        if self.publish(distribution, security_only=True):
+            self.runFinalizeParts(distribution, security_only=True)
 
         if not self.options.security_only:
             self.rsyncBackupDists(distribution)

=== modified file 'lib/lp/archivepublisher/tests/test_publish_ftpmaster.py'
--- lib/lp/archivepublisher/tests/test_publish_ftpmaster.py	2013-07-25 20:25:56 +0000
+++ lib/lp/archivepublisher/tests/test_publish_ftpmaster.py	2014-03-11 03:51:26 +0000
@@ -658,9 +658,23 @@
         distro = script.distributions[0]
         script.setUpDirs()
         script.installDists = FakeMethod()
-        script.publishSecurityUploads(distro)
+        has_published = script.publishSecurityUploads(distro)
+        self.assertFalse(has_published)
         self.assertEqual(0, script.installDists.call_count)
 
+    def test_publishSecurityUploads_returns_true_when_publishes(self):
+        distro = self.makeDistroWithPublishDirectory()
+        distroseries = self.factory.makeDistroSeries(distribution=distro)
+        self.factory.makeSourcePackagePublishingHistory(
+            distroseries=distroseries,
+            pocket=PackagePublishingPocket.SECURITY)
+        script = self.makeScript(distro)
+        script.setUp()
+        script.setUpDirs()
+        script.installDists = FakeMethod()
+        has_published = script.publishSecurityUploads(distro)
+        self.assertTrue(has_published)
+
     def test_publishDistroUploads_publishes_all_distro_archives(self):
         distro = self.makeDistroWithPublishDirectory()
         distroseries = self.factory.makeDistroSeries(distribution=distro)
@@ -721,11 +735,21 @@
 
     def test_security_run_publishes_only_security_updates(self):
         script = self.makeScript(extra_args=['--security-only'])
-        script.publish = FakeMethod()
+        script.runFinalizeParts = FakeMethod()
+        script.publish = FakeMethod(result=True)
         script.main()
         self.assertEqual(1, script.publish.call_count)
         args, kwargs = script.publish.calls[0]
         self.assertEqual({'security_only': True}, kwargs)
+        self.assertEqual(1, script.runFinalizeParts.call_count)
+
+    def test_security_run_empty_security_does_not_finalize(self):
+        script = self.makeScript(extra_args=['--security-only'])
+        script.runFinalizeParts = FakeMethod()
+        script.publish = FakeMethod(result=False)
+        script.main()
+        self.assertEqual(1, script.publish.call_count)
+        self.assertEqual(0, script.runFinalizeParts.call_count)
 
     def test_publishDistroUploads_processes_all_archives(self):
         distro = self.makeDistroWithPublishDirectory()
@@ -786,6 +810,32 @@
                 "Did not find expected marker for %s."
                 % archive.purpose.title)
 
+    def test_publish_always_returns_true_for_primary(self):
+        script = self.makeScript()
+        script.publishDistroUploads = FakeMethod()
+        script.setUp()
+        script.setUpDirs()
+        result = script.publish(script.distributions[0], security_only=False)
+        self.assertTrue(result)
+
+    def test_publish_returns_true_for_non_empty_security(self):
+        script = self.makeScript(extra_args=['--security-only'])
+        script.setUp()
+        script.setUpDirs()
+        script.installDists = FakeMethod()
+        script.publishSecurityUploads = FakeMethod(result=True)
+        result = script.publish(script.distributions[0], security_only=True)
+        self.assertTrue(result)
+
+    def test_publish_returns_false_for_empty_security(self):
+        script = self.makeScript(extra_args=['--security-only'])
+        script.setUp()
+        script.setUpDirs()
+        script.installDists = FakeMethod()
+        script.publishSecurityUploads = FakeMethod(result=False)
+        result = script.publish(script.distributions[0], security_only=True)
+        self.assertFalse(result)
+
     def test_publish_reraises_exception(self):
         # If an Exception comes up while publishing, it bubbles up out
         # of the publish method even though the method must intercept