Merge lp:~jtv/launchpad/bug-779701 into lp:launchpad

[Jeroen T. Vermeulen]

[r=henninge][bug=779701] Create distroseries indexes per suite.

The new, python-based publish-ftpmaster script will create archive indexes for a new distroseries. In the Ubuntu release process this removes a manual "publish-distro.py -A" run for the distroseries. However we want the indexes for each suite in the series created separately, with a separate publish-distro run. This will make it easier to add pockets in the future, for instance. Index creation should be tracked separately for each suite.

This branch makes the index-creation code use the suite name instead of the distroseries name for the marker files that indicate that indexes have been created. It then runs publish-distro separately for each suite.

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

To Q/A:
 * Create a new distroseries for an existing distribution.
 * Run cronscripts/publish-ftpmaster.py -d <distro> -s <series>.
 * See that indexes have been created.
 * Note marker files in the distribution's archive root, one for each suites.

(The marker files are dot files, so may not show up by default).

= 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_publish_ftpmaster.py

[Henning Eggers (henninge)]

Thank you for this nice improvement. All I have to add is this:

- Consider using a local factory method for the frozen distro series that also explains why it needs to be frozen.

- The test naming is inconsistent. You used the right convention for the new tests but left the old ones as they were. Would be nice if you could update those, too, where applicable (test_methodName_condition).

Cheers,
Henning

review: Approve (code)

[Jeroen T. Vermeulen (jtv)]

Thanks. Implemented the changes.

=== modified file 'lib/lp/archivepublisher/scripts/publish_ftpmaster.py'
--- lib/lp/archivepublisher/scripts/publish_ftpmaster.py	2011-05-06 03:10:14 +0000
+++ lib/lp/archivepublisher/scripts/publish_ftpmaster.py	2011-05-09 10:48:36 +0000
@@ -228,19 +228,18 @@
             (archive.purpose, getPubConfig(archive))
             for archive in self.archives)
 
-    def locateIndexesMarker(self, distroseries):
+    def locateIndexesMarker(self, suite):
         """Give path for marker file whose presence marks index creation.
 
-        The file will be created once the archive indexes for
-        `distroseries` have been created.  This is how future runs will
-        know that this work is done.
+        The file will be created once the archive indexes for `suite`
+        have been created.  This is how future runs will know that this
+        work is done.
         """
         archive_root = self.configs[ArchivePurpose.PRIMARY].archiveroot
-        return os.path.join(
-            archive_root, ".created-indexes-for-%s" % distroseries.name)
+        return os.path.join(archive_root, ".created-indexes-for-%s" % suite)
 
-    def needsIndexesCreated(self, distroseries):
-        """Does `distroseries` still need its archive indexes created?
+    def listSuitesNeedingIndexes(self, distroseries):
+        """Find suites in `distroseries` that need indexes created.
 
         Checks for the marker left by `markIndexCreationComplete`.
         """
@@ -249,36 +248,39 @@
             # is in any other state yet has not been marked as having
             # its indexes created, that's because it predates automatic
             # index creation.
-            return False
+            return []
         distro = distroseries.distribution
         publisher_config_set = getUtility(IPublisherConfigSet)
         if publisher_config_set.getByDistribution(distro) is None:
             # We won't be able to do a thing without a publisher config,
             # but that's alright: we have those for all distributions
             # that we want to publish.
-            return False
-        return not file_exists(self.locateIndexesMarker(distroseries))
-
-    def markIndexCreationComplete(self, distroseries):
-        """Note that archive indexes for `distroseries` have been created.
-
-        This tells `needsIndexesCreated` that no, this series no longer needs
-        archive indexes to be set up.
+            return []
+
+        # May need indexes for this series.
+        suites = [
+            distroseries.getSuite(pocket)
+            for pocket in pocketsuffix.iterkeys()]
+        return [
+            suite for suite in suites
+                if not file_exists(self.locateIndexesMarker(suite))]
+
+    def markIndexCreationComplete(self, suite):
+        """Note that archive indexes for `suite` have been created.
+
+        This tells `listSuitesNeedingIndexes` that no, this suite no
+        longer needs archive indexes to be set up.
         """
-        with file(self.locateIndexesMarker(distroseries), "w") as marker:
+        with file(self.locateIndexesMarker(suite), "w") as marker:
             marker.write(
                 "Indexes for %s were created on %s.\n"
-                % (distroseries, datetime.now(utc)))
+                % (suite, datetime.now(utc)))
 
-    def createIndexes(self, distroseries):
+    def createIndexes(self, suite):
         """Create archive indexes for `distroseries`."""
-        self.logger.info(
-            "Creating archive indexes for series %s.", distroseries)
-        suites = [
-            distroseries.getSuite(pocket)
-            for pocket in pocketsuffix.iterkeys()]
-        self.runPublishDistro(args=['-A'], suites=suites)
-        self.markIndexCreationComplete(distroseries)
+        self.logger.info("Creating archive indexes for %s.", suite)
+        self.runPublishDistro(args=['-A'], suites=[suite])
+        self.markIndexCreationComplete(suite)
 
     def processAccepted(self):
         """Run the process-accepted script."""
@@ -556,8 +558,10 @@
         self.recoverWorkingDists()
 
         for series in self.distribution.series:
-            if self.needsIndexesCreated(series):
-                self.createIndexes(series)
+            suites_needing_indexes = self.listSuitesNeedingIndexes(series)
+            if len(suites_needing_indexes) > 0:
+                for suite in suites_needing_indexes:
+                    self.createIndexes(suite)
                 # Don't try to do too much in one run.  Leave the rest
                 # of the work for next time.
                 return

=== modified file 'lib/lp/archivepublisher/tests/test_publish_ftpmaster.py'
--- lib/lp/archivepublisher/tests/test_publish_ftpmaster.py	2011-05-06 03:10:14 +0000
+++ lib/lp/archivepublisher/tests/test_publish_ftpmaster.py	2011-05-09 10:48:36 +0000
@@ -108,6 +108,19 @@
     return file(os.path.join(*path)).read()
 
 
+def get_a_suite(distroseries):
+    """Return some suite name for `distroseries`."""
+    # Don't pick Release; it's too easy.
+    return distroseries.getSuite(PackagePublishingPocket.SECURITY)
+
+
+def get_marker_files(script, distroseries):
+        suites = [
+            distroseries.getSuite(pocket)
+            for pocket in pocketsuffix.iterkeys()]
+        return [script.locateIndexesMarker(suite) for suite in suites]
+
+
 class HelpersMixin:
     """Helpers for the PublishFTPMaster tests."""
 
@@ -769,58 +782,90 @@
 
     def createIndexesMarkerDir(self, script, distroseries):
         """Create the directory for `distroseries`'s indexes marker."""
-        marker = script.locateIndexesMarker(distroseries)
+        marker = script.locateIndexesMarker(get_a_suite(distroseries))
         os.makedirs(os.path.dirname(marker))
 
-    def test_new_frozen_series_needs_indexes_created(self):
+    def makeDistroSeriesNeedingIndexes(self, distribution=None):
+        """Create `DistroSeries` that needs indexes created."""
+        return self.factory.makeDistroSeries(
+            status=SeriesStatus.FROZEN, distribution=distribution)
+
+    def test_listSuitesNeedingIndexes_is_nonempty_for_new_frozen_series(self):
         # If a distroseries is Frozen and has not had its indexes
-        # created yet, needsIndexesCreated returns True for it.
-        series = self.factory.makeDistroSeries(status=SeriesStatus.FROZEN)
-        script = self.makeScript(series.distribution)
-        script.setUp()
-        self.assertTrue(script.needsIndexesCreated(series))
-
-    def test_new_nonfrozen_series_does_not_need_indexes_created(self):
-        # needsIndexesCreated only returns True for Frozen distroseries.
+        # created yet, listSuitesNeedingIndexes returns a nonempty list
+        # for it.
+        series = self.makeDistroSeriesNeedingIndexes()
+        script = self.makeScript(series.distribution)
+        script.setUp()
+        self.assertNotEqual([], list(script.listSuitesNeedingIndexes(series)))
+
+    def test_listSuitesNeedingIndexes_initially_includes_entire_series(self):
+        # If a series has not had any of its indexes created yet,
+        # listSuitesNeedingIndexes returns all of its suites.
+        series = self.makeDistroSeriesNeedingIndexes()
+        script = self.makeScript(series.distribution)
+        script.setUp()
+        self.assertContentEqual(
+            [series.getSuite(pocket) for pocket in pocketsuffix.iterkeys()],
+            script.listSuitesNeedingIndexes(series))
+
+    def test_listSuitesNeedingIndexes_is_empty_for_nonfrozen_series(self):
+        # listSuitesNeedingIndexes only returns suites for Frozen
+        # distroseries.
         series = self.factory.makeDistroSeries()
         script = self.makeScript(series.distribution)
-        self.assertFalse(script.needsIndexesCreated(series))
+        self.assertEqual([], script.listSuitesNeedingIndexes(series))
 
-    def test_distro_without_publisher_config_gets_no_indexes(self):
-        # needsIndexesCreated returns False for distributions that have
-        # no publisher config, such as Debian.  We don't want to publish
-        # these distributions.
-        series = self.factory.makeDistroSeries(status=SeriesStatus.FROZEN)
+    def test_listSuitesNeedingIndexes_is_empty_for_configless_distro(self):
+        # listSuitesNeedingIndexes returns no suites for distributions
+        # that have no publisher config, such as Debian.  We don't want
+        # to publish such distributions.
+        series = self.makeDistroSeriesNeedingIndexes()
         pub_config = get_pub_config(series.distribution)
         IMasterStore(pub_config).remove(pub_config)
         script = self.makeScript(series.distribution)
-        self.assertFalse(script.needsIndexesCreated(series))
+        self.assertEqual([], script.listSuitesNeedingIndexes(series))
 
-    def test_markIndexCreationComplete_tells_needsIndexesCreated_no(self):
-        # The effect of markIndexCreationComplete is to make
-        # needsIndexesCreated for that distroseries return False.
+    def test_markIndexCreationComplete_repels_listSuitesNeedingIndexes(self):
+        # The effect of markIndexCreationComplete is to remove the suite
+        # in question from the results of listSuitesNeedingIndexes for
+        # that distroseries.
         distro = self.makeDistroWithPublishDirectory()
-        series = self.factory.makeDistroSeries(
-            status=SeriesStatus.FROZEN, distribution=distro)
+        series = self.makeDistroSeriesNeedingIndexes(distribution=distro)
         script = self.makeScript(distro)
         script.setUp()
         self.createIndexesMarkerDir(script, series)
 
-        self.assertTrue(script.needsIndexesCreated(series))
-        script.markIndexCreationComplete(series)
-        self.assertFalse(script.needsIndexesCreated(series))
+        needful_suites = script.listSuitesNeedingIndexes(series)
+        suite = get_a_suite(series)
+        script.markIndexCreationComplete(suite)
+        needful_suites.remove(suite)
+        self.assertContentEqual(
+            needful_suites, script.listSuitesNeedingIndexes(series))
+
+    def test_listSuitesNeedingIndexes_ignores_other_series(self):
+        # listSuitesNeedingIndexes only returns suites for series that
+        # need indexes created.  It ignores other distroseries.
+        series = self.makeDistroSeriesNeedingIndexes()
+        self.factory.makeDistroSeries(distribution=series.distribution)
+        script = self.makeScript(series.distribution)
+        script.setUp()
+        suites = list(script.listSuitesNeedingIndexes(series))
+        self.assertNotEqual([], suites)
+        for suite in suites:
+            self.assertThat(suite, StartsWith(series.name))
 
     def test_createIndexes_marks_index_creation_complete(self):
-        # createIndexes calls markIndexCreationComplete for the
-        # distroseries.
+        # createIndexes calls markIndexCreationComplete for the suite.
         distro = self.makeDistroWithPublishDirectory()
         series = self.factory.makeDistroSeries(distribution=distro)
         script = self.makeScript(distro)
         script.markIndexCreationComplete = FakeMethod()
         script.runPublishDistro = FakeMethod()
-        script.createIndexes(series)
+        suite = get_a_suite(series)
+        script.createIndexes(suite)
         self.assertEqual(
-            [((series, ), {})], script.markIndexCreationComplete.calls)
+            [((suite, ), {})], script.markIndexCreationComplete.calls)
 
     def test_failed_index_creation_is_not_marked_complete(self):
         # If index creation fails, it is not marked as having been
@@ -833,7 +878,7 @@
         script.markIndexCreationComplete = FakeMethod()
         script.runPublishDistro = FakeMethod(failure=Boom("Sorry!"))
         try:
-            script.createIndexes(series)
+            script.createIndexes(get_a_suite(series))
         except:
             pass
         self.assertEqual([], script.markIndexCreationComplete.calls)
@@ -846,20 +891,30 @@
         script.setUp()
         archive_root = script.configs[ArchivePurpose.PRIMARY].archiveroot
         self.assertThat(
-            script.locateIndexesMarker(series),
+            script.locateIndexesMarker(get_a_suite(series)),
             StartsWith(os.path.normpath(archive_root)))
 
-    def test_locateIndexesMarker_uses_separate_files_per_series(self):
-        # Different release series of the same distribution get separate
-        # marker files for index creation.
+    def test_locateIndexesMarker_uses_separate_files_per_suite(self):
+        # Each suite in a distroseries gets its own marker file for
+        # index creation.
+        distro = self.makeDistroWithPublishDirectory()
+        series = self.factory.makeDistroSeries(distribution=distro)
+        script = self.makeScript(distro)
+        script.setUp()
+        markers = get_marker_files(script, series)
+        self.assertEqual(sorted(markers), sorted(list(set(markers))))
+
+    def test_locateIndexesMarker_separates_distroseries(self):
+        # Each distroseries gets its own marker files for index
+        # creation.
         distro = self.makeDistroWithPublishDirectory()
         series1 = self.factory.makeDistroSeries(distribution=distro)
         series2 = self.factory.makeDistroSeries(distribution=distro)
         script = self.makeScript(distro)
         script.setUp()
-        self.assertNotEqual(
-            script.locateIndexesMarker(series1),
-            script.locateIndexesMarker(series2))
+        markers1 = set(get_marker_files(script, series1))
+        markers2 = set(get_marker_files(script, series2))
+        self.assertEqual(set(), markers1.intersection(markers2))
 
     def test_locateIndexMarker_uses_hidden_file(self):
         # The index-creation marker file is a "dot file," so it's not
@@ -867,20 +922,23 @@
         series = self.factory.makeDistroSeries()
         script = self.makeScript(series.distribution)
         script.setUp()
+        suite = get_a_suite(series)
         self.assertThat(
-            os.path.basename(script.locateIndexesMarker(series)),
+            os.path.basename(script.locateIndexesMarker(suite)),
             StartsWith("."))
 
     def test_script_calls_createIndexes_for_new_series(self):
         # If the script's main() finds a distroseries that needs its
         # indexes created, it calls createIndexes on that distroseries.
         distro = self.makeDistroWithPublishDirectory()
-        series = self.factory.makeDistroSeries(
-            status=SeriesStatus.FROZEN, distribution=distro)
+        series = self.makeDistroSeriesNeedingIndexes(distribution=distro)
         script = self.makeScript(distro)
         script.createIndexes = FakeMethod()
         script.main()
-        self.assertEqual([((series, ), {})], script.createIndexes.calls)
+        expected_calls = [
+            ((series.getSuite(pocket), ), {})
+            for pocket in pocketsuffix.iterkeys()]
+        self.assertContentEqual(expected_calls, script.createIndexes.calls)
 
     def test_createIndexes_ignores_other_series(self):
         # createIndexes does not accidentally also touch other
@@ -892,14 +950,13 @@
         script.setUp()
         script.runPublishDistro = FakeMethod()
         self.createIndexesMarkerDir(script, series)
+        suite = get_a_suite(series)
 
-        script.createIndexes(series)
+        script.createIndexes(suite)
 
         args, kwargs = script.runPublishDistro.calls[0]
-        suites = kwargs['suites']
-        self.assertEqual(len(pocketsuffix), len(suites))
-        for suite in suites:
-            self.assertThat(suite, StartsWith(series.name))
+        self.assertEqual([suite], kwargs['suites'])
+        self.assertThat(kwargs['suites'][0], StartsWith(series.name))
 
     def test_script_creates_indexes(self):
         # End-to-end test: the script creates indexes for distroseries
@@ -913,7 +970,7 @@
         self.setUpForScriptRun(series.distribution)
         script = self.makeScript(series.distribution)
         script.main()
-        self.assertFalse(script.needsIndexesCreated(series))
+        self.assertEqual([], script.listSuitesNeedingIndexes(series))
         sources = os.path.join(
             script.configs[ArchivePurpose.PRIMARY].distsroot,
             series.name, "main", "source", "Sources")