launchpad-reviewers team mailing list archive

[Raphaël Victor Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Victor Badin has proposed merging lp:~rvb/launchpad/perm-distributionjob-bug-808680 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #808680 in Launchpad itself: "The initializeddistroseries job fails with "permission denied for relation distributionjob""
  https://bugs.launchpad.net/launchpad/+bug/808680

For more details, see:
https://code.launchpad.net/~rvb/launchpad/perm-distributionjob-bug-808680/+merge/68059

This branch fixes a permission error that happens when initializing a series from multiple parents ('ProgrammingError: permission denied for relation distributionjob'). The problem comes from the fact that creating a new publication automatically creates DistroSeriesDifferenceJobs. This branch adds a param to newSourcePublication (lib/lp/soyuz/model/publishing.py) to control the creation of DSD jobs.

= Test =

./bin/test -vvc test_initderiveddistroseries test_multiple_parents_dsd_flag_on

= QA =

Initialize a series from multiple parents with source publications in the second parent. 
-- 
https://code.launchpad.net/~rvb/launchpad/perm-distributionjob-bug-808680/+merge/68059
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/perm-distributionjob-bug-808680 into lp:launchpad.

=== modified file 'lib/lp/registry/tests/test_initderiveddistroseries.py'
--- lib/lp/registry/tests/test_initderiveddistroseries.py	2011-07-08 09:17:32 +0000
+++ lib/lp/registry/tests/test_initderiveddistroseries.py	2011-07-15 09:15:30 +0000
@@ -16,9 +16,13 @@
     LaunchpadZopelessLayer,
     )
 from lp.registry.interfaces.distroseries import DerivationError
+from lp.services.features.testing import FeatureFixture
 from lp.soyuz.interfaces.distributionjob import (
     IInitializeDistroSeriesJobSource,
     )
+from lp.soyuz.model.distroseriesdifferencejob import (
+    FEATURE_FLAG_ENABLE_MODULE,
+    )
 from lp.soyuz.scripts.tests.test_initialize_distroseries import (
     InitializationHelperTestCase,
     )
@@ -95,3 +99,28 @@
         self.assertEquals(
             [(u'p1', u'0.1-1'), (u'p2', u'2.1')],
             binaries)
+
+    def test_multiple_parents_dsd_flag_on(self):
+        # A initialization can happen if the flag for distroseries
+        # difference creation is on.
+        self.useFixture(FeatureFixture({FEATURE_FLAG_ENABLE_MODULE: u'on'}))
+        self.parent1, unused = self.setupParent(
+            packages={})
+        self.parent2, unused = self.setupParent(
+            packages={'p2': '2.1'})
+        child = self.factory.makeDistroSeries()
+        transaction.commit()
+        self.layer.switchDbUser('initializedistroseries')
+
+        child = self._fullInitialize(
+            [self.parent1, self.parent2], child=child)
+        pub_sources = child.main_archive.getPublishedSources(
+            distroseries=child)
+
+        self.assertEqual(1, len(list(pub_sources)))
+        publication = pub_sources[0].sourcepackagerelease
+        self.assertEqual(u'p2', publication.name)
+        self.assertEqual(u'2.1', publication.version)
+        # Switch back to launchpad_main to be able to cleanup the
+        # feature flags.
+        self.layer.switchDbUser('launchpad_main')

=== modified file 'lib/lp/soyuz/interfaces/publishing.py'
--- lib/lp/soyuz/interfaces/publishing.py	2011-06-09 10:50:25 +0000
+++ lib/lp/soyuz/interfaces/publishing.py	2011-07-15 09:15:30 +0000
@@ -83,6 +83,7 @@
     requires manual intervention in the archive.
     """
 
+
 class MissingSymlinkInPool(Exception):
     """Raised when there is a missing symlink in pool.
 
@@ -948,7 +949,8 @@
         """
 
     def newSourcePublication(archive, sourcepackagerelease, distroseries,
-                             component, section, pocket, ancestor):
+                             component, section, pocket, ancestor,
+                             create_dsd_job):
         """Create a new `SourcePackagePublishingHistory`.
 
         :param archive: An `IArchive`
@@ -959,6 +961,8 @@
         :param pocket: A `PackagePublishingPocket`
         :param ancestor: A `ISourcePackagePublishingHistory` for the previous
             version of this publishing record
+        :param create_dsd_job: A boolean indicating whether or not dsd job
+            should be created for the new source publication.
 
         datecreated will be UTC_NOW.
         status will be PackagePublishingStatus.PENDING

=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2011-06-24 12:23:05 +0000
+++ lib/lp/soyuz/model/publishing.py	2011-07-15 09:15:30 +0000
@@ -802,7 +802,8 @@
             section=new_section,
             archive=current.archive)
 
-    def copyTo(self, distroseries, pocket, archive, override=None):
+    def copyTo(self, distroseries, pocket, archive, override=None,
+               create_dsd_job=True):
         """See `ISourcePackagePublishingHistory`."""
         component = self.component
         section = self.section
@@ -817,7 +818,8 @@
             distroseries,
             component,
             section,
-            pocket)
+            pocket,
+            create_dsd_job)
 
     def getStatusSummaryForBuilds(self):
         """See `ISourcePackagePublishingHistory`."""
@@ -1467,7 +1469,7 @@
 
     def newSourcePublication(self, archive, sourcepackagerelease,
                              distroseries, component, section, pocket,
-                             ancestor=None):
+                             ancestor=None, create_dsd_job=True):
         """See `IPublishingSet`."""
         # Avoid circular import.
         from lp.registry.model.distributionsourcepackage import (
@@ -1485,10 +1487,12 @@
             ancestor=ancestor)
         DistributionSourcePackage.ensure(pub)
 
-        if archive == distroseries.main_archive:
-            dsd_job_source = getUtility(IDistroSeriesDifferenceJobSource)
-            dsd_job_source.createForPackagePublication(
-                distroseries, sourcepackagerelease.sourcepackagename, pocket)
+        if create_dsd_job:
+            if archive == distroseries.main_archive:
+                dsd_job_source = getUtility(IDistroSeriesDifferenceJobSource)
+                dsd_job_source.createForPackagePublication(
+                    distroseries, sourcepackagerelease.sourcepackagename,
+                    pocket)
         return pub
 
     def getBuildsForSourceIds(

=== modified file 'lib/lp/soyuz/scripts/initialize_distroseries.py'
--- lib/lp/soyuz/scripts/initialize_distroseries.py	2011-07-07 16:22:20 +0000
+++ lib/lp/soyuz/scripts/initialize_distroseries.py	2011-07-15 09:15:30 +0000
@@ -382,7 +382,8 @@
                         sources_published = do_copy(
                             sources, target_archive, self.distroseries,
                             pocket, include_binaries=not self.rebuild,
-                            check_permissions=False, strict_binaries=False)
+                            check_permissions=False, strict_binaries=False,
+                            create_dsd_job=False)
                         if self.rebuild:
                             for pubrec in sources_published:
                                 pubrec.createMissingBuilds()

=== modified file 'lib/lp/soyuz/scripts/packagecopier.py'
--- lib/lp/soyuz/scripts/packagecopier.py	2011-06-24 08:48:58 +0000
+++ lib/lp/soyuz/scripts/packagecopier.py	2011-07-15 09:15:30 +0000
@@ -525,7 +525,8 @@
 
 def do_copy(sources, archive, series, pocket, include_binaries=False,
             allow_delayed_copies=True, person=None, check_permissions=True,
-            overrides=None, send_email=False, strict_binaries=True):
+            overrides=None, send_email=False, strict_binaries=True,
+            create_dsd_job=True):
     """Perform the complete copy of the given sources incrementally.
 
     Verifies if each copy can be performed using `CopyChecker` and
@@ -559,6 +560,8 @@
     :param send_email: Should we notify for the copy performed?
     :param strict_binaries: If 'include_binaries' is True then setting this
         to True will make the copy fail if binaries cannot be also copied.
+    :param create_dsd_job: A boolean indicating whether or not dsd jobs
+        should be created for the copied source publications.
 
     :raise CannotCopy when one or more copies were not allowed. The error
         will contain the reason why each copy was denied.
@@ -605,7 +608,7 @@
                 override = overrides[overrides_index]
             sub_copies = _do_direct_copy(
                 source, archive, destination_series, pocket,
-                include_binaries, override)
+                include_binaries, override, create_dsd_job)
             if send_email:
                 notify(
                     person, source.sourcepackagerelease, [], [], archive,
@@ -619,7 +622,7 @@
 
 
 def _do_direct_copy(source, archive, series, pocket, include_binaries,
-                    override=None):
+                    override=None, create_dsd_job=True):
     """Copy publishing records to another location.
 
     Copy each item of the given list of `SourcePackagePublishingHistory`
@@ -638,6 +641,8 @@
         not the published binaries for each given source should be also
         copied along with the source.
     :param override: An `IOverride` as per do_copy().
+    :param create_dsd_job: A boolean indicating whether or not a dsd job
+        should be created for the copied source publication.
 
     :return: a list of `ISourcePackagePublishingHistory` and
         `BinaryPackagePublishingHistory` corresponding to the copied
@@ -666,7 +671,8 @@
             assert len(overrides) == 1, (
                 "More than one override encountered, something is wrong.")
             override = overrides[0]
-        source_copy = source.copyTo(series, pocket, archive, override)
+        source_copy = source.copyTo(
+            series, pocket, archive, override, create_dsd_job)
         close_bugs_for_sourcepublication(source_copy)
         copies.append(source_copy)
     else: