← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~wgrant/launchpad/process-accepted-cleanup into lp:launchpad

 

William Grant has proposed merging lp:~wgrant/launchpad/process-accepted-cleanup into lp:launchpad.

Commit message:
Refactor process-accepted's target archive calculation so we can eventually unify them all.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/process-accepted-cleanup/+merge/230201

Refactor process-accepted's target archive calculation so we can eventually unify them all.
-- 
https://code.launchpad.net/~wgrant/launchpad/process-accepted-cleanup/+merge/230201
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/process-accepted-cleanup into lp:launchpad.
=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py	2014-07-18 07:43:06 +0000
+++ lib/lp/soyuz/model/queue.py	2014-08-09 09:23:15 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -439,26 +439,6 @@
                 'Unable to reject queue item due to status.')
         self.status = PassthroughStatusValue(PackageUploadStatus.REJECTED)
 
-    def _closeBugs(self, changesfile_path, logger=None):
-        """Close bugs for a just-accepted source.
-
-        :param changesfile_path: path to the context changesfile.
-        :param logger: optional context Logger object (used on DEBUG level);
-
-        It does not close bugs for PPA sources.
-        """
-        from lp.soyuz.scripts.processaccepted import close_bugs_for_queue_item
-
-        if self.isPPA():
-            debug(logger, "Not closing bugs for PPA source.")
-            return
-
-        debug(logger, "Closing bugs.")
-        changesfile_object = open(changesfile_path, 'r')
-        close_bugs_for_queue_item(
-            self, changesfile_object=changesfile_object)
-        changesfile_object.close()
-
     def _validateBuildsForSource(self, sourcepackagerelease, builds):
         """Check if the sourcepackagerelease generates at least one build.
 
@@ -507,6 +487,8 @@
 
     def acceptFromUploader(self, changesfile_path, logger=None):
         """See `IPackageUpload`."""
+        from lp.soyuz.scripts.processaccepted import close_bugs_for_queue_item
+
         debug(logger, "Setting it to ACCEPTED")
         self.setAccepted()
 
@@ -521,7 +503,9 @@
         [pub_source] = self.realiseUpload()
         builds = pub_source.createMissingBuilds(logger=logger)
         self._validateBuildsForSource(pub_source.sourcepackagerelease, builds)
-        self._closeBugs(changesfile_path, logger)
+        with open(changesfile_path, 'r') as changesfile_object:
+            close_bugs_for_queue_item(
+                self, changesfile_object=changesfile_object)
         self._giveKarma()
 
     def _acceptSyncFromQueue(self):

=== modified file 'lib/lp/soyuz/scripts/processaccepted.py'
--- lib/lp/soyuz/scripts/processaccepted.py	2014-08-05 10:34:11 +0000
+++ lib/lp/soyuz/scripts/processaccepted.py	2014-08-09 09:23:15 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Helper functions for the process-accepted.py script."""
@@ -39,7 +39,6 @@
 from lp.soyuz.interfaces.processacceptedbugsjob import (
     IProcessAcceptedBugsJobSource,
     )
-from lp.soyuz.interfaces.queue import IPackageUploadSet
 from lp.soyuz.model.processacceptedbugsjob import (
     close_bug_ids_for_sourcepackagerelease,
     )
@@ -181,72 +180,6 @@
         job_source.create(distroseries, source_release, bug_ids_to_close)
 
 
-class TargetPolicy:
-    """Policy describing what kinds of archives to operate on."""
-
-    def __init__(self, logger):
-        self.logger = logger
-
-    def getTargetArchives(self, distribution):
-        """Get target archives of the right sort for `distribution`."""
-        raise NotImplemented("getTargetArchives")
-
-    def describeArchive(self, archive):
-        """Return textual description for `archive` in this script run."""
-        raise NotImplemented("describeArchive")
-
-    def postprocessSuccesses(self, queue_ids):
-        """Optionally, post-process successfully processed queue items.
-
-        :param queue_ids: An iterable of `PackageUpload` ids that were
-            successfully processed.
-        """
-
-
-class PPATargetPolicy(TargetPolicy):
-    """Target policy for PPA archives."""
-
-    def getTargetArchives(self, distribution):
-        """See `TargetPolicy`."""
-        return distribution.getPendingAcceptancePPAs()
-
-    def describeArchive(self, archive):
-        """See `TargetPolicy`."""
-        return archive.archive_url
-
-
-class CopyArchiveTargetPolicy(TargetPolicy):
-    """Target policy for copy archives."""
-
-    def getTargetArchives(self, distribution):
-        """See `TargetPolicy`."""
-        return getUtility(IArchiveSet).getArchivesForDistribution(
-            distribution, purposes=[ArchivePurpose.COPY])
-
-    def describeArchive(self, archive):
-        """See `TargetPolicy`."""
-        return archive.displayname
-
-
-class DistroTargetPolicy(TargetPolicy):
-    """Target policy for distro archives."""
-
-    def getTargetArchives(self, distribution):
-        """See `TargetPolicy`."""
-        return distribution.all_distro_archives
-
-    def describeArchive(self, archive):
-        """See `TargetPolicy`."""
-        return archive.purpose.title
-
-    def postprocessSuccesses(self, queue_ids):
-        """See `TargetPolicy`."""
-        self.logger.debug("Closing bugs.")
-        for queue_id in queue_ids:
-            queue_item = getUtility(IPackageUploadSet).get(queue_id)
-            close_bugs_for_queue_item(queue_item)
-
-
 class ProcessAccepted(PublisherScript):
     """Queue/Accepted processor.
 
@@ -281,15 +214,15 @@
             raise OptionValueError(
                 "Can't combine --derived with a distribution name.")
 
-    def makeTargetPolicy(self):
-        """Pick and instantiate a `TargetPolicy` based on given options."""
+    def getTargetArchives(self, distribution):
+        """Find archives to target based on given options."""
         if self.options.ppa:
-            policy_class = PPATargetPolicy
+            return distribution.getPendingAcceptancePPAs()
         elif self.options.copy_archives:
-            policy_class = CopyArchiveTargetPolicy
+            return getUtility(IArchiveSet).getArchivesForDistribution(
+                distribution, purposes=[ArchivePurpose.COPY])
         else:
-            policy_class = DistroTargetPolicy
-        return policy_class(self.logger)
+            return distribution.all_distro_archives
 
     def processQueueItem(self, queue_item):
         """Attempt to process `queue_item`.
@@ -315,22 +248,20 @@
                 "Successfully processed queue item %d", queue_item.id)
             return True
 
-    def processForDistro(self, distribution, target_policy):
+    def processForDistro(self, distribution):
         """Process all queue items for a distribution.
 
         Commits between items.
 
         :param distribution: The `Distribution` to process queue items for.
-        :param target_policy: The applicable `TargetPolicy`.
         :return: A list of all successfully processed items' ids.
         """
         processed_queue_ids = []
-        for archive in target_policy.getTargetArchives(distribution):
-            description = target_policy.describeArchive(archive)
+        for archive in self.getTargetArchives(distribution):
             for distroseries in distribution.series:
 
                 self.logger.debug("Processing queue for %s %s" % (
-                    distroseries.name, description))
+                    archive.reference, distroseries.name))
 
                 queue_items = distroseries.getPackageUploads(
                     status=PackageUploadStatus.ACCEPTED, archive=archive)
@@ -341,21 +272,18 @@
                     # on-disk archive, so the partial state must
                     # make it to the DB.
                     self.txn.commit()
+                    close_bugs_for_queue_item(queue_item)
+                    self.txn.commit()
         return processed_queue_ids
 
     def main(self):
         """Entry point for a LaunchpadScript."""
         self.validateArguments()
-        target_policy = self.makeTargetPolicy()
         try:
             for distro in self.findDistros():
-                queue_ids = self.processForDistro(distro, target_policy)
-                self.txn.commit()
-                target_policy.postprocessSuccesses(queue_ids)
-                self.txn.commit()
-
+                self.processForDistro(distro)
+                self.txn.commit()
         finally:
             self.logger.debug("Rolling back any remaining transactions.")
             self.txn.abort()
-
         return 0

=== modified file 'lib/lp/soyuz/tests/test_processaccepted.py'
--- lib/lp/soyuz/tests/test_processaccepted.py	2014-08-05 10:34:11 +0000
+++ lib/lp/soyuz/tests/test_processaccepted.py	2014-08-09 09:23:15 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test process-accepted.py"""
@@ -157,9 +157,11 @@
                     upload for upload in uploads
                     if upload.package_upload.status ==
                         PackageUploadStatus.DONE])
-                self.assertEqual(
-                    min(len(uploads), inner_self.commit_count),
-                    done_count)
+                # We actually commit twice for each upload: once for the
+                # queue item itself, and again to close its bugs.
+                self.assertIn(
+                    min(len(uploads) * 2, inner_self.commit_count),
+                    (done_count * 2, (done_count * 2) - 1))
 
         script = self.getScript([])
         switch_dbuser(self.dbuser)


Follow ups