launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-836743 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #836743 in Launchpad itself: "Publisher breaks on archives without publisher config"
  https://bugs.launchpad.net/launchpad/+bug/836743

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-836743/+merge/73267

= Summary =

The publish-ftpmaster script broke when it encountered a distribution archive without a publisher config.  Could be a bit of a handicap when iterating over all derived distributions, including any that haven't had their configuration set up.


== Proposed fix ==

Have getPubConfig return None for archives that have no configuration in the database.  Have publish_ftpmaster ignore distros without archive configs.


== Pre-implementation notes ==

This came up while trying out the publisher's --all-derived option on dogfood.  Apparently the publisher config needs to be set up in the UI; distros that don't have it set up yet evidently aren't ready for publishing.


== Implementation details ==

You'll note that I made getConfigs() skip missing configs _per archive_, but main() check for and skip missing configs _per distro_.  A distro can have multiple archives.

But the publisher config in the database is keyed off the distro's main archive.  So there seems to be no middle ground between "distro without configs" and "distro with configs," unless it's a distro without archives.


== Tests ==

{{{
./bin/test -vvc lp.archivepublisher.tests.test_publish_ftpmaster
}}}


== Demo and Q/A ==

Run "cronscripts/publish-ftpmaster.py --all-derived" on dogfood.  It should get further than the previous attempt, which broke after a few seconds (i.e. ZCML processing, sigh) during getConfigs very early in the script.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/archivepublisher/scripts/publish_ftpmaster.py
  lib/lp/archivepublisher/tests/test_config.py
  lib/lp/archivepublisher/config.py
  lib/lp/archivepublisher/tests/test_publish_ftpmaster.py
-- 
https://code.launchpad.net/~jtv/launchpad/bug-836743/+merge/73267
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-836743 into lp:launchpad.

=== modified file 'lib/lp/archivepublisher/config.py'
--- lib/lp/archivepublisher/config.py	2011-03-30 04:51:49 +0000
+++ lib/lp/archivepublisher/config.py	2011-08-29 17:16:29 +0000
@@ -13,8 +13,8 @@
 from canonical.config import config
 from lp.archivepublisher.interfaces.publisherconfig import IPublisherConfigSet
 from lp.soyuz.enums import (
+    archive_suffixes,
     ArchivePurpose,
-    archive_suffixes,
     )
 
 
@@ -32,6 +32,8 @@
     ppa_config = config.personalpackagearchive
     db_pubconf = getUtility(
         IPublisherConfigSet).getByDistribution(archive.distribution)
+    if db_pubconf is None:
+        return None
 
     pubconf.temproot = os.path.join(
         db_pubconf.root_dir, '%s-temp' % archive.distribution.name)

=== modified file 'lib/lp/archivepublisher/scripts/publish_ftpmaster.py'
--- lib/lp/archivepublisher/scripts/publish_ftpmaster.py	2011-08-22 08:03:13 +0000
+++ lib/lp/archivepublisher/scripts/publish_ftpmaster.py	2011-08-29 17:16:29 +0000
@@ -148,6 +148,21 @@
         return None
 
 
+def map_distro_pubconfigs(distro):
+    """Return dict mapping archive purpose for distro's pub configs.
+
+    :param distro: `Distribution` to get publisher configs for.
+    :return: Dict mapping archive purposes to publisher configs, insofar as
+        they have publisher configs.
+    """
+    candidates = [
+        (archive.purpose, getPubConfig(archive))
+        for archive in get_publishable_archives(distro)]
+    return dict(
+        (purpose, config)
+        for purpose, config in candidates if config is not None)
+
+
 class PublishFTPMaster(LaunchpadCronScript):
     """Publish a distro (update).
 
@@ -241,9 +256,7 @@
         So: getConfigs[distro][purpose] gives you a config.
         """
         return dict(
-            (distro, dict(
-                (archive.purpose, getPubConfig(archive))
-                for archive in get_publishable_archives(distro)))
+            (distro, map_distro_pubconfigs(distro))
             for distro in self.distributions)
 
     def locateIndexesMarker(self, distribution, suite):
@@ -536,9 +549,10 @@
         """Publish the distro's complete uploads."""
         self.logger.debug("Full publication.  This may take some time.")
         for archive in get_publishable_archives(distribution):
-            # This, for the main archive, is where the script spends
-            # most of its time.
-            self.publishDistroArchive(distribution, archive)
+            if archive.purpose in self.configs[distribution]:
+                # This, for the main archive, is where the script spends
+                # most of its time.
+                self.publishDistroArchive(distribution, archive)
 
     def publish(self, distribution, security_only=False):
         """Do the main publishing work.
@@ -622,5 +636,6 @@
         self.setUpDirs()
 
         for distribution in self.distributions:
-            self.processDistro(distribution)
-            self.txn.commit()
+            if len(self.configs[distribution]) > 0:
+                self.processDistro(distribution)
+                self.txn.commit()

=== added file 'lib/lp/archivepublisher/tests/test_config.py'
--- lib/lp/archivepublisher/tests/test_config.py	1970-01-01 00:00:00 +0000
+++ lib/lp/archivepublisher/tests/test_config.py	2011-08-29 17:16:29 +0000
@@ -0,0 +1,19 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test publisher configs handling."""
+
+__metaclass__ = type
+
+from canonical.testing.layers import ZopelessDatabaseLayer
+from lp.archivepublisher.config import getPubConfig
+from lp.testing import TestCaseWithFactory
+
+
+class TestGetPubConfig(TestCaseWithFactory):
+
+    layer = ZopelessDatabaseLayer
+
+    def test_getPubConfig_returns_None_if_no_publisherconfig_found(self):
+        archive = self.factory.makeDistribution(no_pubconf=True).main_archive
+        self.assertEqual(None, getPubConfig(archive))

=== modified file 'lib/lp/archivepublisher/tests/test_publish_ftpmaster.py'
--- lib/lp/archivepublisher/tests/test_publish_ftpmaster.py	2011-08-19 02:34:11 +0000
+++ lib/lp/archivepublisher/tests/test_publish_ftpmaster.py	2011-08-29 17:16:29 +0000
@@ -310,10 +310,20 @@
         script.setUp()
         self.assertEqual([distro], script.getConfigs().keys())
 
+    def test_getConfigs_skips_configless_distros(self):
+        distro = self.factory.makeDistribution(no_pubconf=True)
+        script = self.makeScript(distro)
+        script.setUp()
+        self.assertEqual({}, script.getConfigs()[distro])
+
     def test_script_is_happy_with_no_publications(self):
         distro = self.makeDistroWithPublishDirectory()
         self.makeScript(distro).main()
 
+    def test_script_is_happy_with_no_pubconfigs(self):
+        distro = self.factory.makeDistribution(no_pubconf=True)
+        self.makeScript(distro).main()
+
     def test_produces_listings(self):
         distro = self.makeDistroWithPublishDirectory()
         self.makeScript(distro).main()