launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05734
[Merge] lp:~jtv/launchpad/bug-849683-patchup into lp:launchpad
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-849683-patchup into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #849683 in Launchpad itself: "{B,S}PPH Populator are temporary"
https://bugs.launchpad.net/launchpad/+bug/849683
For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-849683-patchup/+merge/83729
= Summary =
We recently finished populating two new denormalized database columns:
SourcePackagePublishingHistory.sourcepackagename
BinaryPackagePublishingHistory.binarypackagename
It's time to put NOT NULL constraints in place, to make sure we never forget to initialize them. Once that is done, we can start making use of the new columns.
However a few tests were still breaking.
== Proposed fix ==
I ran the test suite with the constraints in place, and found a fair number of broken tests. Most of the breakage was caused by neglect on the part of the Soyuz test publisher, but some tests also had their own factory methods. Finally, the package cloner (which I'm not sure we actually use, to be honest) also created some rows in direct SQL but failed to initialize the column.
This branch fixes up all cases I could find, including a pair of cases in a test where the mistake happened not to break the test.
== Pre-implementation notes ==
We're still debating in what order to apply the various next steps. But ensuring that all code initializes the new fields appropriately should be uncontroversial; in fact one would have expected it to have been taken care of before starting to initialize.
== Implementation details ==
You'll notice some lint fixes. I also reformatted some SQL so that I could see the structure more clearly. This helps me make sure that I'm not reading the nesting of parentheses etc. wrong. Don't want to make changes in code I can't read first!
== Tests ==
The failing tests were:
{{{
lib/lp/registry/doc/distroseries.txt
lib/canonical/launchpad/doc/publishing-security.txt
canonical.launchpad.utilities.ftests.test_gpghandler.TestImportKeyRing.testHomeDirectoryJob
lib/lp/app/doc/tales.txt
lib/lp/registry/browser/tests/distributionsourcepackage-views.txt
lib/lp/registry/doc/sourcepackage.txt
lib/lp/soyuz/browser/tests/archive-views.txt
lib/lp/soyuz/browser/tests/builder-views.txt
lib/lp/soyuz/browser/tests/distributionsourcepackagerelease-views.txt
lib/lp/soyuz/browser/tests/distroseriesqueue-views.txt
lib/lp/soyuz/browser/tests/publishing-views.txt
lib/lp/soyuz/doc/archive-deletion.txt
lib/lp/soyuz/doc/archive-dependencies.txt
lib/lp/soyuz/doc/archive.txt
lib/lp/soyuz/doc/binarypackagerelease.txt
lib/lp/soyuz/doc/build-files.txt
lib/lp/soyuz/doc/buildd-queuebuilder-lookup.txt
lib/lp/soyuz/doc/distribution.txt
lib/lp/soyuz/doc/distroarchseries.txt
lib/lp/soyuz/doc/distroseries-publishing-lookups.txt
lib/lp/soyuz/doc/distroseriesqueue-translations.txt
lib/lp/soyuz/doc/distroseriesqueue.txt
lib/lp/soyuz/doc/package-diff.txt
lib/lp/soyuz/doc/package-meta-classes.txt
lib/lp/soyuz/doc/packageupload-lookups.txt
lib/lp/soyuz/doc/sourcepackagerelease.txt
lib/lp/archivepublisher/tests/deathrow.txt
}}}
== Demo and Q/A ==
There's not much to demo or Q/A. Just keep watching the ackee garbo-hourly logs _after_ deployment, to see if the BPPH/SPPH populator finds any new rows to initialize. They shouldn't. Then again, they haven't found any for almost a week now without this branch in place.
One other thing we can do some time after deployment is:
{{{
-- Should count 0 rows.
SELECT count(*) AS incorrect_spphs
FROM SourcePackagePublishingHistory SPPH
JOIN SourcePackageRelease AS SPR ON SPR.id = SPPH.sourcepackagerelease
WHERE SPPH.sourcepackagename <> SPR.sourcepackagename;
-- Should count 0 rows.
SELECT count(*) AS incorrect_bpphs
FROM BinaryPackagePublishingHistory BPPH
JOIN BinaryPackageRelease AS BPR ON BPR.id = BPPH.binarypackagerelease
WHERE BPPH.binarypackagename <> BPR.binarypackagename;
}}}
However any rows that this might turn up would previously have shown up in the garbo-hourly logs as rows that needed populating.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/soyuz/tests/ppa.py
lib/lp/soyuz/doc/distroseriesqueue-translations.txt
lib/lp/soyuz/tests/test_publishing.py
lib/lp/soyuz/tests/soyuz.py
lib/lp/soyuz/model/packagecloner.py
lib/lp/soyuz/doc/buildd-queuebuilder-lookup.txt
--
https://code.launchpad.net/~jtv/launchpad/bug-849683-patchup/+merge/83729
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-849683-patchup into lp:launchpad.
=== modified file 'lib/lp/soyuz/doc/buildd-queuebuilder-lookup.txt'
--- lib/lp/soyuz/doc/buildd-queuebuilder-lookup.txt 2011-06-28 15:04:29 +0000
+++ lib/lp/soyuz/doc/buildd-queuebuilder-lookup.txt 2011-11-29 05:36:27 +0000
@@ -1,4 +1,5 @@
-= Build Candidates Lookup =
+Build Candidates Lookup
+=======================
IDistroSeries.getSourcesPublishedForAllArchives method is used by the
@@ -47,7 +48,8 @@
... sourcepackagerelease=the_spr, component=the_spr.component,
... section=the_spr.section, datecreated=UTC_NOW,
... distroseries=distroseries, status=status,
- ... pocket=pocket, archive=archive)
+ ... pocket=pocket, archive=archive,
+ ... sourcepackagename=the_spr.sourcepackagename)
Create a collection of source publishing records in different
archives, pockets and statuses. This function creates:
@@ -59,36 +61,36 @@
* 1 PUBLISHED and 1 PENDING records in RELEASE pocket, COPY archive;
>>> def create_sample_publications(distroseries):
- ... create_publication(
- ... distroseries, PackagePublishingPocket.RELEASE,
- ... distroseries.main_archive, PackagePublishingStatus.PUBLISHED)
- ... create_publication(
- ... distroseries, PackagePublishingPocket.RELEASE,
- ... distroseries.main_archive, PackagePublishingStatus.PENDING)
- ... create_publication(
- ... distroseries, PackagePublishingPocket.UPDATES,
- ... distroseries.main_archive, PackagePublishingStatus.PUBLISHED)
- ... create_publication(
- ... distroseries, PackagePublishingPocket.UPDATES,
- ... distroseries.main_archive, PackagePublishingStatus.PENDING)
- ... create_publication(
- ... distroseries, PackagePublishingPocket.RELEASE,
- ... cprov.archive, PackagePublishingStatus.PUBLISHED)
- ... create_publication(
- ... distroseries, PackagePublishingPocket.RELEASE,
- ... cprov.archive, PackagePublishingStatus.PENDING)
- ... create_publication(
- ... distroseries, PackagePublishingPocket.RELEASE,
- ... partner_archive, PackagePublishingStatus.PUBLISHED)
- ... create_publication(
- ... distroseries, PackagePublishingPocket.RELEASE,
- ... partner_archive, PackagePublishingStatus.PENDING)
- ... create_publication(
- ... distroseries, PackagePublishingPocket.RELEASE,
- ... copy_archive, PackagePublishingStatus.PUBLISHED)
- ... create_publication(
- ... distroseries, PackagePublishingPocket.RELEASE,
- ... copy_archive, PackagePublishingStatus.PENDING)
+ ... create_publication(
+ ... distroseries, PackagePublishingPocket.RELEASE,
+ ... distroseries.main_archive, PackagePublishingStatus.PUBLISHED)
+ ... create_publication(
+ ... distroseries, PackagePublishingPocket.RELEASE,
+ ... distroseries.main_archive, PackagePublishingStatus.PENDING)
+ ... create_publication(
+ ... distroseries, PackagePublishingPocket.UPDATES,
+ ... distroseries.main_archive, PackagePublishingStatus.PUBLISHED)
+ ... create_publication(
+ ... distroseries, PackagePublishingPocket.UPDATES,
+ ... distroseries.main_archive, PackagePublishingStatus.PENDING)
+ ... create_publication(
+ ... distroseries, PackagePublishingPocket.RELEASE,
+ ... cprov.archive, PackagePublishingStatus.PUBLISHED)
+ ... create_publication(
+ ... distroseries, PackagePublishingPocket.RELEASE,
+ ... cprov.archive, PackagePublishingStatus.PENDING)
+ ... create_publication(
+ ... distroseries, PackagePublishingPocket.RELEASE,
+ ... partner_archive, PackagePublishingStatus.PUBLISHED)
+ ... create_publication(
+ ... distroseries, PackagePublishingPocket.RELEASE,
+ ... partner_archive, PackagePublishingStatus.PENDING)
+ ... create_publication(
+ ... distroseries, PackagePublishingPocket.RELEASE,
+ ... copy_archive, PackagePublishingStatus.PUBLISHED)
+ ... create_publication(
+ ... distroseries, PackagePublishingPocket.RELEASE,
+ ... copy_archive, PackagePublishingStatus.PENDING)
This function will store previous publishing records that will be
ignored later when inspecting the example results (sampledata already
@@ -124,7 +126,8 @@
... inspect_new_candidates(distroseries)
-== CURRENT DistroSeries ==
+CURRENT DistroSeries
+--------------------
For a distroseries in CURRENT state, which means STABLE, we expect to
retrieve the following publications:
@@ -156,7 +159,8 @@
PENDING RELEASE PARTNER
-== DEVELOPMENT DistroSeries ==
+DEVELOPMENT DistroSeries
+------------------------
For a distroseries in DEVELOPMENT state, which means UNSTABLE, we
expect to retrieve the following publications:
=== modified file 'lib/lp/soyuz/doc/distroarchseriesbinarypackage.txt'
--- lib/lp/soyuz/doc/distroarchseriesbinarypackage.txt 2011-08-28 08:36:14 +0000
+++ lib/lp/soyuz/doc/distroarchseriesbinarypackage.txt 2011-11-29 05:36:27 +0000
@@ -69,6 +69,7 @@
>>> pe = BinaryPackagePublishingHistory(
... binarypackagerelease=bpr.id,
+ ... binarypackagename=bpr.binarypackagename,
... component=main_component.id,
... section=misc_section.id,
... priority=priority,
@@ -112,6 +113,7 @@
>>> pe = BinaryPackagePublishingHistory(
... binarypackagerelease=bpr.id,
+ ... binarypackagename=bpr.binarypackagename,
... component=main_component.id,
... section=misc_section.id,
... priority=priority,
=== modified file 'lib/lp/soyuz/doc/distroseriesqueue-translations.txt'
--- lib/lp/soyuz/doc/distroseriesqueue-translations.txt 2011-07-25 13:16:52 +0000
+++ lib/lp/soyuz/doc/distroseriesqueue-translations.txt 2011-11-29 05:36:27 +0000
@@ -64,6 +64,7 @@
>>> publishing_history = SourcePackagePublishingHistory(
... distroseries=dapper.id,
... sourcepackagerelease=source_package_release.id,
+ ... sourcepackagename=source_package_release.sourcepackagename,
... component=source_package_release.component.id,
... section=source_package_release.section.id,
... status=PackagePublishingStatus.PUBLISHED,
=== modified file 'lib/lp/soyuz/model/packagecloner.py'
--- lib/lp/soyuz/model/packagecloner.py 2011-07-25 13:56:43 +0000
+++ lib/lp/soyuz/model/packagecloner.py 2011-11-29 05:36:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Logic for bulk copying of source/binary publishing history data."""
@@ -184,19 +184,29 @@
INSERT INTO BinaryPackagePublishingHistory (
binarypackagerelease, distroarchseries, status,
component, section, priority, archive, datecreated,
- datepublished, pocket)
- SELECT bpph.binarypackagerelease, %s as distroarchseries,
- bpph.status, bpph.component, bpph.section, bpph.priority,
- %s as archive, %s as datecreated, %s as datepublished,
- %s as pocket
+ datepublished, pocket, binarypackagename)
+ SELECT
+ bpph.binarypackagerelease,
+ %s as distroarchseries,
+ bpph.status,
+ bpph.component,
+ bpph.section,
+ bpph.priority,
+ %s as archive,
+ %s as datecreated,
+ %s as datepublished,
+ %s as pocket,
+ bpph.binarypackagename
""" % sqlvalues(
destination_das, destination.archive, UTC_NOW, UTC_NOW,
destination.pocket)
query += clause_tables
query += """
- WHERE bpph.distroarchseries = %s AND bpph.status in (%s, %s)
- AND
- bpph.pocket = %s and bpph.archive = %s
+ WHERE
+ bpph.distroarchseries = %s AND
+ bpph.status in (%s, %s) AND
+ bpph.pocket = %s AND
+ bpph.archive = %s
""" % sqlvalues(
origin_das,
PackagePublishingStatus.PENDING,
@@ -424,14 +434,25 @@
query = '''
INSERT INTO SourcePackagePublishingHistory (
sourcepackagerelease, distroseries, status, component,
- section, archive, datecreated, datepublished, pocket)
- SELECT spph.sourcepackagerelease, %s as distroseries,
- spph.status, spph.component, spph.section, %s as archive,
- %s as datecreated, %s as datepublished,
- %s as pocket
+ section, archive, datecreated, datepublished, pocket,
+ sourcepackagename)
+ SELECT
+ spph.sourcepackagerelease,
+ %s as distroseries,
+ spph.status,
+ spph.component,
+ spph.section,
+ %s as archive,
+ %s as datecreated,
+ %s as datepublished,
+ %s as pocket,
+ spph.sourcepackagename
FROM SourcePackagePublishingHistory AS spph
- WHERE spph.distroseries = %s AND spph.status in (%s, %s) AND
- spph.pocket = %s and spph.archive = %s
+ WHERE
+ spph.distroseries = %s AND
+ spph.status in (%s, %s) AND
+ spph.pocket = %s AND
+ spph.archive = %s
''' % sqlvalues(
destination.distroseries, destination.archive, UTC_NOW,
UTC_NOW, destination.pocket, origin.distroseries,
@@ -440,24 +461,27 @@
origin.pocket, origin.archive)
if sourcepackagenames and len(sourcepackagenames) > 0:
- query += '''AND spph.sourcepackagerelease IN (
- (SELECT spr.id
- FROM SourcePackageRelease AS spr,
- SourcePackageName AS spn
- WHERE spr.sourcepackagename = spn.id
- AND spn.name IN %s))''' % sqlvalues(
- sourcepackagenames)
+ query += '''
+ AND spph.sourcepackagerelease IN (
+ SELECT spr.id
+ FROM SourcePackageRelease AS spr, SourcePackageName AS spn
+ WHERE
+ spr.sourcepackagename = spn.id AND spn.name IN %s
+ )''' % sqlvalues(sourcepackagenames)
if origin.packagesets:
- query += '''AND spph.sourcepackagerelease IN
- (SELECT spr.id
- FROM SourcePackageRelease AS spr,
- packagesetsources AS pss,
- flatpackagesetinclusion AS fpsi
- WHERE spr.sourcepackagename
- = pss.sourcepackagename
- AND pss.packageset = fpsi.child
- AND fpsi.parent in %s)
+ query += '''
+ AND spph.sourcepackagerelease IN (
+ SELECT spr.id
+ FROM
+ SourcePackageRelease AS spr,
+ packagesetsources AS pss,
+ flatpackagesetinclusion AS fpsi
+ WHERE
+ spr.sourcepackagename = pss.sourcepackagename AND
+ pss.packageset = fpsi.child AND
+ fpsi.parent in %s
+ )
''' % sqlvalues([p.id for p in origin.packagesets])
if origin.component:
=== modified file 'lib/lp/soyuz/tests/ppa.py'
--- lib/lp/soyuz/tests/ppa.py 2010-08-24 15:29:01 +0000
+++ lib/lp/soyuz/tests/ppa.py 2011-11-29 05:36:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Helper functions/classes to be used when testing Personal Package Archives.
@@ -13,12 +13,12 @@
from lp.registry.interfaces.person import IPersonSet
from lp.registry.interfaces.pocket import PackagePublishingPocket
from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
-from lp.soyuz.interfaces.binarypackagename import IBinaryPackageNameSet
-from lp.soyuz.interfaces.component import IComponentSet
from lp.soyuz.enums import (
PackagePublishingPriority,
PackagePublishingStatus,
)
+from lp.soyuz.interfaces.binarypackagename import IBinaryPackageNameSet
+from lp.soyuz.interfaces.component import IComponentSet
from lp.soyuz.model.binarypackagerelease import BinaryPackageRelease
from lp.soyuz.model.publishing import (
BinaryPackagePublishingHistory,
@@ -103,6 +103,7 @@
SourcePackagePublishingHistory(
distroseries=distroseries,
sourcepackagerelease=sourcepackagerelease,
+ sourcepackagename=sourcepackagename,
component=main_component,
section=sourcepackagerelease.section,
status=publishing_status,
@@ -120,6 +121,7 @@
distroarchseries = distroseries[arch]
BinaryPackagePublishingHistory(
binarypackagerelease=binarypackagerelease,
+ binarypackagename=binarypackagename,
distroarchseries=distroarchseries,
component=main_component,
section=sourcepackagerelease.section,
=== modified file 'lib/lp/soyuz/tests/soyuz.py'
--- lib/lp/soyuz/tests/soyuz.py 2011-09-06 08:35:10 +0000
+++ lib/lp/soyuz/tests/soyuz.py 2011-11-29 05:36:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Helper functions/classes for Soyuz tests."""
@@ -16,16 +16,14 @@
from canonical.config import config
from canonical.launchpad.database.librarian import LibraryFileAlias
-from canonical.launchpad.ftests import (
- import_public_test_keys,
- )
+from canonical.launchpad.ftests import import_public_test_keys
from canonical.launchpad.testing.fakepackager import FakePackager
from canonical.testing.layers import LaunchpadZopelessLayer
from lp.registry.interfaces.distribution import IDistributionSet
from lp.registry.interfaces.person import IPersonSet
from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.soyuz.enums import PackagePublishingStatus
from lp.soyuz.interfaces.packagediff import IPackageDiffSet
-from lp.soyuz.enums import PackagePublishingStatus
from lp.soyuz.model.publishing import (
BinaryPackagePublishingHistory,
SourcePackagePublishingHistory,
@@ -96,6 +94,7 @@
for status, archive, pocket in self.sample_publishing_data:
pub = SourcePackagePublishingHistory(
sourcepackagerelease=sourcepackagerelease,
+ sourcepackagename=sourcepackagerelease.sourcepackagename,
distroseries=distroseries,
component=sourcepackagerelease.component,
section=sourcepackagerelease.section,
@@ -119,6 +118,7 @@
for status, archive, pocket in self.sample_publishing_data:
pub = BinaryPackagePublishingHistory(
binarypackagerelease=binarypackagerelease,
+ binarypackagename=binarypackagerelease.binarypackagename,
distroarchseries=distroarchseries,
component=binarypackagerelease.component,
section=binarypackagerelease.section,
=== modified file 'lib/lp/soyuz/tests/test_publishing.py'
--- lib/lp/soyuz/tests/test_publishing.py 2011-11-21 04:48:45 +0000
+++ lib/lp/soyuz/tests/test_publishing.py 2011-11-29 05:36:27 +0000
@@ -272,6 +272,7 @@
spph = SourcePackagePublishingHistory(
distroseries=distroseries,
sourcepackagerelease=spr,
+ sourcepackagename=spr.sourcepackagename,
component=spr.component,
section=spr.section,
status=status,
@@ -459,6 +460,7 @@
pub = BinaryPackagePublishingHistory(
distroarchseries=arch,
binarypackagerelease=binarypackagerelease,
+ binarypackagename=binarypackagerelease.binarypackagename,
component=binarypackagerelease.component,
section=binarypackagerelease.section,
priority=binarypackagerelease.priority,
