launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-848954 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #848954 in Launchpad itself: "ztm.commit may break gina's dry-run mode"
  https://bugs.launchpad.net/launchpad/+bug/848954

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-848954/+merge/75188

= 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
-- 
https://code.launchpad.net/~jtv/launchpad/bug-848954/+merge/75188
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-848954 into lp:launchpad.

=== modified file 'lib/lp/soyuz/scripts/gina/handlers.py'
--- lib/lp/soyuz/scripts/gina/handlers.py	2011-09-08 09:27:01 +0000
+++ lib/lp/soyuz/scripts/gina/handlers.py	2011-09-13 14:14:34 +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:14:34 +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()
 
@@ -78,8 +73,8 @@
         elif not q:
             return None
         else:
-            raise AssertionError, "%s killed us on %s %s" \
-                % (len(q), query, args)
+            raise AssertionError(
+                "%s killed us on %s %s" % (len(q), query, args))
 
     def _exec(self, query, args=None):
         #print repr(query), repr(args)
@@ -93,12 +88,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 +119,7 @@
                                                 architecture.id
                                       AND    arch_string = %s""",
                                         (name, version, arch))
+
     def getSections(self):
         return self._query("""SELECT section FROM section""")
 
@@ -134,4 +135,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:14:34 +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.