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

[Jeroen T. Vermeulen]

[r=gmb][bug=848954] Remove Gina's untested, unappreciated dry-run mode.

= Summary =

Gina has a dry-run mode.

This turned out to be a big surprise to both William and Julian, the people I most expected to be familiar with it. So I'll say it again to avoid any confusion: Gina has a dry-run mode.

Is it useful? That depends on whether it works. Does it work? We don't know. Are there any tests to prove that it works? We know there aren't. Breaking dry-run mode, as I did earlier, did not break any tests.

== Proposed fix ==

Rather than fixing and testing dry-run mode (the test would involve an extra, expensive script run), and if necessary fixing it further for not good reason, we chose to ditch dry-run mode.

We can add it back later if there's any interest in it, but hopefully by then we'll be able to do it right: instantiate a Gina script object in-process, set up some test objects, run the script object, and verify that no changes were committed.

== Pre-implementation notes ==

The salient bit:

(20:18:00) bigjools: first question, do we need a dry run mode?
(20:18:14) bigjools: IOW, how does it help us?
(20:18:34) jtv: Well since young Will here had no idea it even existed, I was wondering whether you might know the answer.
(20:19:00) bigjools: I don't I'm afraid, the Gina code has been barely touched in years, well before I started working on LP

== Implementation details ==

I fixed some lint as well. Sorry for the diff pollution.

== Tests ==

{{{
./bin/test -vvc lp.soyuz -t gina -t katie
}}}

== Demo and Q/A ==

We're getting quite experienced at doing gina test runs!

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/scripts/gina/katie.py
  scripts/gina.py
  lib/lp/soyuz/scripts/gina/handlers.py

./scripts/gina.py
      26: '_pythonpath' imported but unused

[Graham Binns (gmb)]

[1]

71 + raise AssertionError(
72 + "%s killed us on %s %s" % (len(q), query, args))

I know you didn't write it, but this error message makes little sense to me... any mileage in changing it in this branch, or does it make sufficient sense to those In The Know that it should remain untouched?

review: Approve (code)

=== modified file 'lib/lp/soyuz/scripts/gina/handlers.py'
--- lib/lp/soyuz/scripts/gina/handlers.py	2011-09-13 04:31:22 +0000
+++ lib/lp/soyuz/scripts/gina/handlers.py	2011-09-13 14:48:25 +0000
@@ -165,9 +165,8 @@
     This class is used to handle the import process.
     """
 
-    def __init__(self, ztm, distro_name, distroseries_name, dry_run,
-                 ktdb, archive_root, keyrings, pocket, component_override):
-        self.dry_run = dry_run
+    def __init__(self, ztm, distro_name, distroseries_name, ktdb,
+                 archive_root, keyrings, pocket, component_override):
         self.pocket = pocket
         self.component_override = component_override
         self.ztm = ztm
@@ -191,13 +190,11 @@
 
     def commit(self):
         """Commit to the database."""
-        if not self.dry_run:
-            self.ztm.commit()
+        self.ztm.commit()
 
     def abort(self):
         """Rollback changes to the database."""
-        if not self.dry_run:
-            self.ztm.abort()
+        self.ztm.abort()
 
     def ensure_archinfo(self, archtag):
         """Append retrived distroarchseries info to a dict."""
 
=== modified file 'lib/lp/soyuz/scripts/gina/katie.py'
--- lib/lp/soyuz/scripts/gina/katie.py	2011-09-06 05:00:26 +0000
+++ lib/lp/soyuz/scripts/gina/katie.py	2011-09-13 14:48:25 +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).
 
 __metaclass__ = type
@@ -15,10 +15,9 @@
 
 
 class Katie:
-    def __init__(self, dbname, suite, dry_run):
+    def __init__(self, dbname, suite):
         self.suite = suite
         self.dbname = dbname
-        self.dry_run = dry_run
         log.info("Connecting to %s as %s" % (dbname, config.gina.dbuser))
         self.db = connect(user=config.gina.dbuser, dbname=dbname)
 
@@ -39,10 +38,6 @@
             return s
 
     def commit(self):
-        if self.dry_run:
-            # Not committing -- we're on a dry run
-            log.debug("Not committing (dry run)")
-            return
         log.debug("Committing")
         return self.db.commit()
 
@@ -65,7 +60,6 @@
         return self._get_dicts(cursor)
 
     def _query(self, query, args=None):
-        #print repr(query), repr(args)
         cursor = self.db.cursor()
         cursor.execute(query, args or [])
         results = cursor.fetchall()
@@ -78,11 +72,11 @@
         elif not q:
             return None
         else:
-            raise AssertionError, "%s killed us on %s %s" \
-                % (len(q), query, args)
+            raise AssertionError(
+                "Expected 0 or 1 result from this query, but got %d: "
+                "'%s' (with arguments: %s)." % (len(q), query, args))
 
     def _exec(self, query, args=None):
-        #print repr(query), repr(args)
         cursor = self.db.cursor()
         cursor.execute(query, args or [])
         return cursor
@@ -93,12 +87,17 @@
 
     def getSourcePackageRelease(self, name, version):
         log.debug("Hunting for release %s / %s" % (name, version))
-        ret =  self._query_to_dict("""SELECT * FROM source, fingerprint
-                                      WHERE  source = %s
-                                      AND    source.sig_fpr = fingerprint.id
-                                      AND    version = %s""", (name, version))
+        ret = self._query_to_dict("""
+            SELECT *
+            FROM source, fingerprint
+            WHERE
+                source = %s AND
+                source.sig_fpr = fingerprint.id AND
+                version = %s""",
+            (name, version))
         if not ret:
-            log.debug("that spr didn't turn up. Attempting to find via ubuntu")
+            log.debug(
+                "that spr didn't turn up. Attempting to find via ubuntu")
         else:
             return ret
 
@@ -119,6 +118,7 @@
                                                 architecture.id
                                       AND    arch_string = %s""",
                                         (name, version, arch))
+
     def getSections(self):
         return self._query("""SELECT section FROM section""")
 
@@ -134,4 +134,3 @@
            AND override.package = %s
            AND suite.suite_name = %s
         """, (sourcepackage, self.suite))[0]
-
 
=== modified file 'scripts/gina.py'
--- scripts/gina.py	2011-09-13 08:44:05 +0000
+++ scripts/gina.py	2011-09-13 14:48:25 +0000
@@ -87,8 +87,6 @@
     source_only = target_section.source_only
     spnames_only = target_section.sourcepackagenames_only
 
-    dry_run = options.dry_run
-
     KTDB = target_section.katie_dbname
 
     LIBRHOST = config.librarian.upload_host
@@ -107,7 +105,6 @@
     log.info("SourcePackage Only: %s", source_only)
     log.info("SourcePackageName Only: %s", spnames_only)
     log.debug("Librarian: %s:%s", LIBRHOST, LIBRPORT)
-    log.info("Dry run: %s", dry_run)
     log.info("")
 
     if not hasattr(PackagePublishingPocket, pocket.upper()):
@@ -126,7 +123,7 @@
     kdb = None
     keyrings = None
     if KTDB:
-        kdb = Katie(KTDB, distroseries, dry_run)
+        kdb = Katie(KTDB, distroseries)
         keyrings = _get_keyring(keyrings_root)
 
     try:
@@ -140,8 +137,8 @@
 
     packages_map = PackagesMap(arch_component_items)
     importer_handler = ImporterHandler(
-        ztm, distro, distroseries, dry_run, kdb, package_root, keyrings,
-        pocket, component_override)
+        ztm, distro, distroseries, kdb, package_root, keyrings, pocket,
+        component_override)
 
     import_sourcepackages(
         packages_map, kdb, package_root, keyrings, importer_handler)
@@ -321,9 +318,6 @@
         self.parser.add_option("-l", "--list-targets", action="store_true",
             help="List configured import targets", dest="list_targets",
             default=False)
-        self.parser.add_option("-n", "--dry-run", action="store_true",
-            help="Don't commit changes to the database",
-            dest="dry_run", default=False)
 
     def getConfiguredTargets(self):
         """Get the configured import targets.