Merge lp:~julian-edwards/launchpad/ppa-expire-sources into lp:launchpad

[Julian Edwards]

[Julian Edwards (julian-edwards)]

= Summary =
Expire superseded PPA source files

== Proposed fix ==
This change extends the existing script that expires PPA binaries so that it
expires sources as well.

== Pre-implementation notes ==
None, I suck. Besides, it's trivial.

== Implementation details ==
Fairly straightforward extension of the existing script so that it injects
librarian IDs for sources into the loop that was deleting just the binaries.

My only question to you, my reviewer, is to ask if you can come up with a
decent refactoring of the two nearly identical SQL strings. I tried a bunch
of things but only succeeded in making it totally unreadable.

Finally, the script name is now slightly bogus, but that can be fixed some
other time. The LOSAs are desperate for more librarian space.

== Tests ==
bin/test -vvct test_expire_ppa_bins

== Demo and Q/A ==

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  database/schema/security.cfg
  lib/lp/soyuz/scripts/tests/test_expire_ppa_bins.py
  lib/lp/soyuz/scripts/expire_ppa_binaries.py

[Brad Crittenden (bac)]

Hi Julian,

The branch looks good. You mentioned renaming the file but deferred due to time constraints. Seems like it would take 10 seconds so you might as well do it now. expire_ppa_resources.py ?

As far as refactoring the SQL it looks like you could use common aliases and then many of your comparisons would be the same, leaving you to just customize the WHERE clauses. Would that help? Or, if it was Storm-i-fied you may find it easier to refactor. Just two lame suggestions.

Otherwise your change looks very well done. Thanks.

review: Approve (code)

[Julian Edwards (julian-edwards)]

On Tuesday 19 January 2010 19:30:24 Brad Crittenden wrote:
> Review: Approve code
> Hi Julian,
>
> The branch looks good. You mentioned renaming the file but deferred due to
> time constraints. Seems like it would take 10 seconds so you might as
> well do it now. expire_ppa_resources.py ?

I would have done that if it were not for the fact that the losas also need to
change their crontabs, which makes it a >10 second job :) But still, we'll
take the hit and do it. Thanks for prodding me.

> As far as refactoring the SQL it looks like you could use common aliases
> and then many of your comparisons would be the same, leaving you to just
> customize the WHERE clauses. Would that help?

I tried. It was really unreadable and the refactoring didn't help reduce the
size of the code that much.

> Or, if it was Storm-i-fied
> you may find it easier to refactor. Just two lame suggestions.

I'd love to do that but it's quite hard, STORM doesn't have the EXCEPT
command.

> Otherwise your change looks very well done. Thanks.

Thanks, I'll land this soon!

=== renamed file 'cronscripts/expire-ppa-binaries.py' => 'cronscripts/expire-ppa-files.py'
--- cronscripts/expire-ppa-binaries.py	2009-10-13 14:38:07 +0000
+++ cronscripts/expire-ppa-files.py	2010-01-20 10:35:29 +0000
@@ -17,6 +17,6 @@
 
 if __name__ == '__main__':
     script = PPABinaryExpirer(
-        'expire-ppa-binaries', dbuser=config.binaryfile_expire.dbuser)
+        'expire-ppa-files', dbuser=config.binaryfile_expire.dbuser)
     script.lock_and_run()
 
 
=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2010-01-06 09:06:56 +0000
+++ database/schema/security.cfg	2010-01-20 10:35:29 +0000
@@ -1612,6 +1612,9 @@
 public.person                           = SELECT
 public.libraryfilealias                 = SELECT, UPDATE
 public.securebinarypackagepublishinghistory = SELECT
+public.sourcepackagereleasefile         = SELECT
+public.sourcepackagepublishinghistory   = SELECT
+public.sourcepackagerelease             = SELECT
 
 [create-merge-proposals]
 type=user
 
=== modified file 'lib/lp/soyuz/scripts/expire_ppa_binaries.py'
--- lib/lp/soyuz/scripts/expire_ppa_binaries.py	2009-12-11 14:35:28 +0000
+++ lib/lp/soyuz/scripts/expire_ppa_binaries.py	2010-01-20 10:35:29 +0000
@@ -54,7 +54,61 @@
             help=("The number of days after which to expire binaries. "
                   "Must be specified."))
 
-    def determineExpirables(self, num_days):
+    def determineSourceExpirables(self, num_days):
+        """Return expirable libraryfilealias IDs."""
+        # Avoid circular imports.
+        from lp.soyuz.interfaces.archive import ArchivePurpose
+
+        stay_of_execution = '%d days' % num_days
+
+        # The subquery here has to repeat the checks for privacy and
+        # blacklisting on *other* publications that are also done in
+        # the main loop for the archive being considered.
+        results = self.store.execute("""
+            SELECT lfa.id
+            FROM
+                LibraryFileAlias AS lfa,
+                Archive,
+                SourcePackageReleaseFile AS sprf,
+                SourcePackageRelease AS spr,
+                SourcePackagePublishingHistory AS spph
+            WHERE
+                lfa.id = sprf.libraryfile
+                AND spr.id = sprf.sourcepackagerelease
+                AND spph.sourcepackagerelease = spr.id
+                AND spph.dateremoved < (
+                    CURRENT_TIMESTAMP AT TIME ZONE 'UTC' - interval %s)
+                AND spph.archive = archive.id
+                AND archive.purpose = %s
+                AND lfa.expires IS NULL
+            EXCEPT
+            SELECT sprf.libraryfile
+            FROM
+                SourcePackageRelease AS spr,
+                SourcePackageReleaseFile AS sprf,
+                SourcePackagePublishingHistory AS spph,
+                Archive AS a,
+                Person AS p
+            WHERE
+                spr.id = sprf.sourcepackagerelease
+                AND spph.sourcepackagerelease = spr.id
+                AND spph.archive = a.id
+                AND p.id = a.owner
+                AND (
+                    p.name IN %s
+                    OR a.private IS TRUE
+                    OR a.purpose != %s
+                    OR dateremoved >
+                        CURRENT_TIMESTAMP AT TIME ZONE 'UTC' - interval %s
+                    OR dateremoved IS NULL);
+            """ % sqlvalues(
+                stay_of_execution, ArchivePurpose.PPA, self.blacklist,
+                ArchivePurpose.PPA, stay_of_execution))
+
+        lfa_ids = results.get_all()
+        return lfa_ids
+
+    def determineBinaryExpirables(self, num_days):
         """Return expirable libraryfilealias IDs."""
         # Avoid circular imports.
         from lp.soyuz.interfaces.archive import ArchivePurpose
@@ -116,7 +170,8 @@
         self.store = getUtility(IStoreSelector).get(
             MAIN_STORE, DEFAULT_FLAVOR)
 
-        lfa_ids = self.determineExpirables(num_days)
+        lfa_ids = self.determineSourceExpirables(num_days)
+        lfa_ids.extend(self.determineBinaryExpirables(num_days))
         batch_count = 0
         batch_limit = 500
         for id in lfa_ids:
 
=== modified file 'lib/lp/soyuz/scripts/tests/test_expire_ppa_bins.py'
--- lib/lp/soyuz/scripts/tests/test_expire_ppa_bins.py	2009-12-11 14:35:28 +0000
+++ lib/lp/soyuz/scripts/tests/test_expire_ppa_bins.py	2010-01-20 10:35:29 +0000
@@ -74,54 +74,76 @@
         self.layer.switchDbUser(self.dbuser)
         script.main()
 
-    def assertExpired(self, publication):
-        self.assertNotEqual(
-            publication.binarypackagerelease.files[0].libraryfile.expires,
-            None,
-            "lfa.expires should be set, but it's not.")
-
-    def assertNotExpired(self, publication):
-        self.assertEqual(
-            publication.binarypackagerelease.files[0].libraryfile.expires,
+    def assertBinaryExpired(self, publication):
+        self.assertNotEqual(
+            publication.binarypackagerelease.files[0].libraryfile.expires,
+            None,
+            "lfa.expires should be set, but it's not.")
+
+    def assertBinaryNotExpired(self, publication):
+        self.assertEqual(
+            publication.binarypackagerelease.files[0].libraryfile.expires,
+            None,
+            "lfa.expires should be None, but it's not.")
+
+    def assertSourceExpired(self, publication):
+        self.assertNotEqual(
+            publication.sourcepackagerelease.files[0].libraryfile.expires,
+            None,
+            "lfa.expires should be set, but it's not.")
+
+    def assertSourceNotExpired(self, publication):
+        self.assertEqual(
+            publication.sourcepackagerelease.files[0].libraryfile.expires,
             None,
             "lfa.expires should be None, but it's not.")
 
     def testNoExpirationWithNoDateremoved(self):
         """Test that no expiring happens if no dateremoved set."""
         pkg1 = self.stp.getPubSource(
-            sourcename="pkg1", architecturehintlist="i386", archive=self.ppa)
+            sourcename="pkg1", architecturehintlist="i386", archive=self.ppa,
+            dateremoved=None)
         [pub] = self.stp.getPubBinaries(
             pub_source=pkg1, dateremoved=None, archive=self.ppa)
 
         self.runScript()
-        self.assertNotExpired(pub)
+        self.assertSourceNotExpired(pkg1)
+        self.assertBinaryNotExpired(pub)
 
     def testNoExpirationWithDateUnderThreshold(self):
         """Test no expiring if dateremoved too recent."""
         pkg2 = self.stp.getPubSource(
-            sourcename="pkg2", architecturehintlist="i386", archive=self.ppa)
+            sourcename="pkg2", architecturehintlist="i386", archive=self.ppa,
+            dateremoved=self.under_threshold_date)
         [pub] = self.stp.getPubBinaries(
             pub_source=pkg2, dateremoved=self.under_threshold_date,
             archive=self.ppa)
 
         self.runScript()
-        self.assertNotExpired(pub)
+        self.assertSourceNotExpired(pkg2)
+        self.assertBinaryNotExpired(pub)
 
     def testExpirationWithDateOverThreshold(self):
         """Test expiring works if dateremoved old enough."""
         pkg3 = self.stp.getPubSource(
-            sourcename="pkg3", architecturehintlist="i386", archive=self.ppa)
+            sourcename="pkg3", architecturehintlist="i386", archive=self.ppa,
+            dateremoved=self.over_threshold_date)
         [pub] = self.stp.getPubBinaries(
             pub_source=pkg3, dateremoved=self.over_threshold_date,
             archive=self.ppa)
 
         self.runScript()
-        self.assertExpired(pub)
+        self.assertSourceExpired(pkg3)
+        self.assertBinaryExpired(pub)
 
     def testNoExpirationWithDateOverThresholdAndOtherValidPublication(self):
         """Test no expiry if dateremoved old enough but other publication."""
         pkg4 = self.stp.getPubSource(
-            sourcename="pkg4", architecturehintlist="i386", archive=self.ppa)
+            sourcename="pkg4", architecturehintlist="i386", archive=self.ppa,
+            dateremoved=self.over_threshold_date)
+        other_source = pkg4.copyTo(
+            pkg4.distroseries, pkg4.pocket, self.ppa2)
+        other_source.secure_record.dateremoved = None
         [pub] = self.stp.getPubBinaries(
             pub_source=pkg4, dateremoved=self.over_threshold_date,
             archive=self.ppa)
@@ -130,16 +152,21 @@
         other_binary.secure_record.dateremoved = None
 
         self.runScript()
-        self.assertNotExpired(pub)
+        self.assertSourceNotExpired(pkg4)
+        self.assertBinaryNotExpired(pub)
 
     def testNoExpirationWithDateOverThresholdAndOtherPubUnderThreshold(self):
         """Test no expiring.
-        
+
         Test no expiring if dateremoved old enough but other publication
         not over date threshold.
         """
         pkg5 = self.stp.getPubSource(
-            sourcename="pkg5", architecturehintlist="i386", archive=self.ppa)
+            sourcename="pkg5", architecturehintlist="i386", archive=self.ppa,
+            dateremoved=self.over_threshold_date)
+        other_source = pkg5.copyTo(
+            pkg5.distroseries, pkg5.pocket, self.ppa2)
+        other_source.secure_record.dateremoved = self.under_threshold_date
         [pub] = self.stp.getPubBinaries(
             pub_source=pkg5, dateremoved=self.over_threshold_date,
             archive=self.ppa)
@@ -148,67 +175,77 @@
         other_binary.secure_record.dateremoved = self.under_threshold_date
 
         self.runScript()
-        self.assertNotExpired(pub)
+        self.assertSourceNotExpired(pkg5)
+        self.assertBinaryNotExpired(pub)
 
     def _setUpExpirablePublications(self, archive=None):
         """Helper to set up two publications that are both expirable."""
         if archive is None:
             archive = self.ppa
         pkg5 = self.stp.getPubSource(
-            sourcename="pkg5", architecturehintlist="i386", archive=archive)
+            sourcename="pkg5", architecturehintlist="i386", archive=archive,
+            dateremoved=self.over_threshold_date)
+        other_source = pkg5.copyTo(
+            pkg5.distroseries, pkg5.pocket, self.ppa2)
+        other_source.secure_record.dateremoved = self.over_threshold_date
         [pub] = self.stp.getPubBinaries(
             pub_source=pkg5, dateremoved=self.over_threshold_date,
             archive=archive)
         [other_binary] = pub.copyTo(
             pub.distroarchseries.distroseries, pub.pocket, self.ppa2)
         other_binary.secure_record.dateremoved = self.over_threshold_date
-        return pub
+        return pkg5, pub
 
     def testNoExpirationWithDateOverThresholdAndOtherPubOverThreshold(self):
         """Test expiring works.
-        
+
         Test expiring works if dateremoved old enough and other publication
         is over date threshold.
         """
-        pub = self._setUpExpirablePublications()
+        source, binary = self._setUpExpirablePublications()
         self.runScript()
-        self.assertExpired(pub)
+        self.assertSourceExpired(source)
+        self.assertBinaryExpired(binary)
 
     def testBlacklistingWorks(self):
         """Test that blacklisted PPAs are not expired."""
-        pub = self._setUpExpirablePublications()
+        source, binary = self._setUpExpirablePublications()
         script = self.getScript()
         script.blacklist = ["cprov",]
         self.layer.txn.commit()
         self.layer.switchDbUser(self.dbuser)
         script.main()
-        self.assertNotExpired(pub)
+        self.assertSourceNotExpired(source)
+        self.assertBinaryNotExpired(binary)
 
     def testPrivatePPAsNotExpired(self):
         """Test that private PPAs are not expired."""
         self.ppa.private = True
         self.ppa.buildd_secret = "foo"
-        pub = self._setUpExpirablePublications()
+        source, binary = self._setUpExpirablePublications()
         self.runScript()
-        self.assertNotExpired(pub)
+        self.assertSourceNotExpired(source)
+        self.assertBinaryNotExpired(binary)
 
     def testDryRun(self):
         """Test that when dryrun is specified, nothing is expired."""
-        pub = self._setUpExpirablePublications()
+        source, binary = self._setUpExpirablePublications()
         # We have to commit here otherwise when the script aborts it
         # will remove the test publications we just created.
         self.layer.txn.commit()
         script = self.getScript(['--dry-run'])
         self.layer.switchDbUser(self.dbuser)
         script.main()
-        self.assertNotExpired(pub)
+        self.assertSourceNotExpired(source)
+        self.assertBinaryNotExpired(binary)
 
     def testDoesNotAffectNonPPA(self):
         """Test that expiry does not happen for non-PPA publications."""
         ubuntu_archive = getUtility(IDistributionSet)['ubuntu'].main_archive
-        pub = self._setUpExpirablePublications(ubuntu_archive)
+        source, binary = self._setUpExpirablePublications(ubuntu_archive)
         self.runScript()
-        self.assertNotExpired(pub)
+        self.assertSourceNotExpired(source)
+        self.assertBinaryNotExpired(binary)
 
 
 def test_suite():