launchpad-reviewers team mailing list archive

[Colin Watson <mp+416113@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:refactor-get-disk-pool into launchpad:master.

Commit message:
Push DiskPool construction down to Config.getDiskPool

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/416113

This eliminates some largely-duplicated code, and will potentially also make it a little easier to substitute alternative implementations of `DiskPool` for some archives.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:refactor-get-disk-pool into launchpad:master.

diff --git a/lib/lp/archivepublisher/config.py b/lib/lp/archivepublisher/config.py
index de45925..92c3917 100644
--- a/lib/lp/archivepublisher/config.py
+++ b/lib/lp/archivepublisher/config.py
@@ -6,10 +6,12 @@
 # to managing the archive publisher's configuration as stored in the
 # distribution and distroseries tables
 
+import logging
 import os
 
 from zope.component import getUtility
 
+from lp.archivepublisher.diskpool import DiskPool
 from lp.archivepublisher.interfaces.publisherconfig import IPublisherConfigSet
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.services.config import config
@@ -150,3 +152,23 @@ class Config:
                 continue
             if not os.path.exists(directory):
                 os.makedirs(directory, 0o755)
+
+    def getDiskPool(self, log, pool_root_override=None):
+        """Return a DiskPool instance for this publisher configuration.
+
+        It ensures the given archive location matches the minimal structure
+        required.
+
+        :param log: A logger.
+        :param pool_root_override: Use this pool root for the archive
+            instead of the one provided by the publishing configuration.
+        """
+        log.debug("Preparing on-disk pool representation.")
+        dp_log = logging.getLogger("DiskPool")
+        pool_root = pool_root_override
+        if pool_root is None:
+            pool_root = self.poolroot
+        dp = DiskPool(pool_root, self.temproot, dp_log)
+        # Set the diskpool's log level to INFO to suppress debug output.
+        dp_log.setLevel(logging.INFO)
+        return dp
diff --git a/lib/lp/archivepublisher/deathrow.py b/lib/lp/archivepublisher/deathrow.py
index 87722a3..b540c80 100644
--- a/lib/lp/archivepublisher/deathrow.py
+++ b/lib/lp/archivepublisher/deathrow.py
@@ -6,7 +6,6 @@ Processes removals of packages that are scheduled for deletion.
 """
 
 import datetime
-import logging
 import os
 
 import pytz
@@ -19,7 +18,6 @@ from storm.locals import (
     )
 
 from lp.archivepublisher.config import getPubConfig
-from lp.archivepublisher.diskpool import DiskPool
 from lp.services.database.constants import UTC_NOW
 from lp.services.database.interfaces import IStore
 from lp.services.librarian.model import (
@@ -57,19 +55,10 @@ def getDeathRow(archive, log, pool_root_override):
     log.debug("Grab publisher config.")
     pubconf = getPubConfig(archive)
 
-    if (pool_root_override is not None and
-        archive.purpose == ArchivePurpose.PRIMARY):
-        pool_root = pool_root_override
-    else:
-        pool_root = pubconf.poolroot
+    if archive.purpose != ArchivePurpose.PRIMARY:
+        pool_root_override = None
 
-    log.debug("Preparing on-disk pool representation.")
-
-    diskpool_log = logging.getLogger("DiskPool")
-    # Set the diskpool's log level to INFO to suppress debug output
-    diskpool_log.setLevel(20)
-
-    dp = DiskPool(pool_root, pubconf.temproot, diskpool_log)
+    dp = pubconf.getDiskPool(log, pool_root_override=pool_root_override)
 
     log.debug("Preparing death row.")
     return DeathRow(archive, dp, log)
diff --git a/lib/lp/archivepublisher/publishing.py b/lib/lp/archivepublisher/publishing.py
index a5a0cc4..9204a7a 100644
--- a/lib/lp/archivepublisher/publishing.py
+++ b/lib/lp/archivepublisher/publishing.py
@@ -24,7 +24,6 @@ from itertools import (
     chain,
     groupby,
     )
-import logging
 import lzma
 from operator import attrgetter
 import os
@@ -45,7 +44,6 @@ from zope.interface import (
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.archivepublisher import HARDCODED_COMPONENT_ORDER
 from lp.archivepublisher.config import getPubConfig
-from lp.archivepublisher.diskpool import DiskPool
 from lp.archivepublisher.domination import Dominator
 from lp.archivepublisher.indices import (
     build_binary_stanza_fields,
@@ -145,21 +143,6 @@ def get_suffixed_indices(path):
     return {path + suffix for suffix in ('', '.gz', '.bz2', '.xz')}
 
 
-def _getDiskPool(pubconf, log):
-    """Return a DiskPool instance for a given PubConf.
-
-    It ensures the given archive location matches the minimal structure
-    required.
-    """
-    log.debug("Preparing on-disk pool representation.")
-    dp = DiskPool(pubconf.poolroot, pubconf.temproot,
-                  logging.getLogger("DiskPool"))
-    # Set the diskpool's log level to INFO to suppress debug output
-    dp.logger.setLevel(logging.INFO)
-
-    return dp
-
-
 def getPublisher(archive, allowed_suites, log, distsroot=None):
     """Return an initialized Publisher instance for the given context.
 
@@ -174,7 +157,7 @@ def getPublisher(archive, allowed_suites, log, distsroot=None):
                   % archive.owner.name)
     pubconf = getPubConfig(archive)
 
-    disk_pool = _getDiskPool(pubconf, log)
+    disk_pool = pubconf.getDiskPool(log)
 
     if distsroot is not None:
         log.debug("Overriding dists root with %s." % distsroot)
diff --git a/lib/lp/archivepublisher/tests/test_config.py b/lib/lp/archivepublisher/tests/test_config.py
index 37a57dc..ec11bfa 100644
--- a/lib/lp/archivepublisher/tests/test_config.py
+++ b/lib/lp/archivepublisher/tests/test_config.py
@@ -6,6 +6,7 @@
 Publisher configuration provides archive-dependent filesystem paths.
 """
 
+import logging
 import os
 
 from zope.component import getUtility
@@ -13,6 +14,7 @@ from zope.component import getUtility
 from lp.archivepublisher.config import getPubConfig
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.services.config import config
+from lp.services.log.logger import BufferLogger
 from lp.soyuz.enums import ArchivePurpose
 from lp.soyuz.interfaces.archive import IArchiveSet
 from lp.testing import TestCaseWithFactory
@@ -106,6 +108,21 @@ class TestGetPubConfig(TestCaseWithFactory):
         self.assertIs(None, copy_config.metaroot)
         self.assertIs(None, copy_config.stagingroot)
 
+    def test_getDiskPool(self):
+        primary_config = getPubConfig(self.ubuntutest.main_archive)
+        disk_pool = primary_config.getDiskPool(BufferLogger())
+        self.assertEqual(self.root + "/ubuntutest/pool/", disk_pool.rootpath)
+        self.assertEqual(self.root + "/ubuntutest-temp/", disk_pool.temppath)
+        self.assertEqual(logging.INFO, disk_pool.logger.level)
+
+    def test_getDiskPool_pool_root_override(self):
+        primary_config = getPubConfig(self.ubuntutest.main_archive)
+        disk_pool = primary_config.getDiskPool(
+            BufferLogger(), pool_root_override="/path/to/pool")
+        self.assertEqual("/path/to/pool/", disk_pool.rootpath)
+        self.assertEqual(self.root + "/ubuntutest-temp/", disk_pool.temppath)
+        self.assertEqual(logging.INFO, disk_pool.logger.level)
+
 
 class TestGetPubConfigPPA(TestCaseWithFactory):