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

Proposed by Jeroen T. Vermeulen on 2011-09-13
Status: Merged
Approved by: Jeroen T. Vermeulen on 2011-09-13
Approved revision: no longer in the source branch.
Merged at revision: 13951
Proposed branch: lp:~jtv/launchpad/bug-848954
Merge into: lp:launchpad
Diff against target: 174 lines (+23/-33)
3 files modified
lib/lp/soyuz/scripts/gina/handlers.py (+4/-7)
lib/lp/soyuz/scripts/gina/katie.py (+16/-17)
scripts/gina.py (+3/-9)
To merge this branch: bzr merge lp:~jtv/launchpad/bug-848954
Reviewer Review Type Date Requested Status
Graham Binns (community) code 2011-09-13 Approve on 2011-09-13
Review via email: mp+75188@code.launchpad.net

Commit message

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

Description of the change

= 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

To post a comment you must log in.
Graham Binns (gmb) wrote :

[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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/soyuz/scripts/gina/handlers.py'
2--- lib/lp/soyuz/scripts/gina/handlers.py 2011-09-13 04:31:22 +0000
3+++ lib/lp/soyuz/scripts/gina/handlers.py 2011-09-13 14:48:25 +0000
4@@ -165,9 +165,8 @@
5 This class is used to handle the import process.
6 """
7
8- def __init__(self, ztm, distro_name, distroseries_name, dry_run,
9- ktdb, archive_root, keyrings, pocket, component_override):
10- self.dry_run = dry_run
11+ def __init__(self, ztm, distro_name, distroseries_name, ktdb,
12+ archive_root, keyrings, pocket, component_override):
13 self.pocket = pocket
14 self.component_override = component_override
15 self.ztm = ztm
16@@ -191,13 +190,11 @@
17
18 def commit(self):
19 """Commit to the database."""
20- if not self.dry_run:
21- self.ztm.commit()
22+ self.ztm.commit()
23
24 def abort(self):
25 """Rollback changes to the database."""
26- if not self.dry_run:
27- self.ztm.abort()
28+ self.ztm.abort()
29
30 def ensure_archinfo(self, archtag):
31 """Append retrived distroarchseries info to a dict."""
32
33=== modified file 'lib/lp/soyuz/scripts/gina/katie.py'
34--- lib/lp/soyuz/scripts/gina/katie.py 2011-09-06 05:00:26 +0000
35+++ lib/lp/soyuz/scripts/gina/katie.py 2011-09-13 14:48:25 +0000
36@@ -1,4 +1,4 @@
37-# Copyright 2009 Canonical Ltd. This software is licensed under the
38+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
39 # GNU Affero General Public License version 3 (see the file LICENSE).
40
41 __metaclass__ = type
42@@ -15,10 +15,9 @@
43
44
45 class Katie:
46- def __init__(self, dbname, suite, dry_run):
47+ def __init__(self, dbname, suite):
48 self.suite = suite
49 self.dbname = dbname
50- self.dry_run = dry_run
51 log.info("Connecting to %s as %s" % (dbname, config.gina.dbuser))
52 self.db = connect(user=config.gina.dbuser, dbname=dbname)
53
54@@ -39,10 +38,6 @@
55 return s
56
57 def commit(self):
58- if self.dry_run:
59- # Not committing -- we're on a dry run
60- log.debug("Not committing (dry run)")
61- return
62 log.debug("Committing")
63 return self.db.commit()
64
65@@ -65,7 +60,6 @@
66 return self._get_dicts(cursor)
67
68 def _query(self, query, args=None):
69- #print repr(query), repr(args)
70 cursor = self.db.cursor()
71 cursor.execute(query, args or [])
72 results = cursor.fetchall()
73@@ -78,11 +72,11 @@
74 elif not q:
75 return None
76 else:
77- raise AssertionError, "%s killed us on %s %s" \
78- % (len(q), query, args)
79+ raise AssertionError(
80+ "Expected 0 or 1 result from this query, but got %d: "
81+ "'%s' (with arguments: %s)." % (len(q), query, args))
82
83 def _exec(self, query, args=None):
84- #print repr(query), repr(args)
85 cursor = self.db.cursor()
86 cursor.execute(query, args or [])
87 return cursor
88@@ -93,12 +87,17 @@
89
90 def getSourcePackageRelease(self, name, version):
91 log.debug("Hunting for release %s / %s" % (name, version))
92- ret = self._query_to_dict("""SELECT * FROM source, fingerprint
93- WHERE source = %s
94- AND source.sig_fpr = fingerprint.id
95- AND version = %s""", (name, version))
96+ ret = self._query_to_dict("""
97+ SELECT *
98+ FROM source, fingerprint
99+ WHERE
100+ source = %s AND
101+ source.sig_fpr = fingerprint.id AND
102+ version = %s""",
103+ (name, version))
104 if not ret:
105- log.debug("that spr didn't turn up. Attempting to find via ubuntu")
106+ log.debug(
107+ "that spr didn't turn up. Attempting to find via ubuntu")
108 else:
109 return ret
110
111@@ -119,6 +118,7 @@
112 architecture.id
113 AND arch_string = %s""",
114 (name, version, arch))
115+
116 def getSections(self):
117 return self._query("""SELECT section FROM section""")
118
119@@ -134,4 +134,3 @@
120 AND override.package = %s
121 AND suite.suite_name = %s
122 """, (sourcepackage, self.suite))[0]
123-
124
125=== modified file 'scripts/gina.py'
126--- scripts/gina.py 2011-09-13 08:44:05 +0000
127+++ scripts/gina.py 2011-09-13 14:48:25 +0000
128@@ -87,8 +87,6 @@
129 source_only = target_section.source_only
130 spnames_only = target_section.sourcepackagenames_only
131
132- dry_run = options.dry_run
133-
134 KTDB = target_section.katie_dbname
135
136 LIBRHOST = config.librarian.upload_host
137@@ -107,7 +105,6 @@
138 log.info("SourcePackage Only: %s", source_only)
139 log.info("SourcePackageName Only: %s", spnames_only)
140 log.debug("Librarian: %s:%s", LIBRHOST, LIBRPORT)
141- log.info("Dry run: %s", dry_run)
142 log.info("")
143
144 if not hasattr(PackagePublishingPocket, pocket.upper()):
145@@ -126,7 +123,7 @@
146 kdb = None
147 keyrings = None
148 if KTDB:
149- kdb = Katie(KTDB, distroseries, dry_run)
150+ kdb = Katie(KTDB, distroseries)
151 keyrings = _get_keyring(keyrings_root)
152
153 try:
154@@ -140,8 +137,8 @@
155
156 packages_map = PackagesMap(arch_component_items)
157 importer_handler = ImporterHandler(
158- ztm, distro, distroseries, dry_run, kdb, package_root, keyrings,
159- pocket, component_override)
160+ ztm, distro, distroseries, kdb, package_root, keyrings, pocket,
161+ component_override)
162
163 import_sourcepackages(
164 packages_map, kdb, package_root, keyrings, importer_handler)
165@@ -321,9 +318,6 @@
166 self.parser.add_option("-l", "--list-targets", action="store_true",
167 help="List configured import targets", dest="list_targets",
168 default=False)
169- self.parser.add_option("-n", "--dry-run", action="store_true",
170- help="Don't commit changes to the database",
171- dest="dry_run", default=False)
172
173 def getConfiguredTargets(self):
174 """Get the configured import targets.