Merge lp:~jtv/launchpad/bug-436426-2 into lp:launchpad
- bug-436426-2
- Merge into devel
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Edwin Grubbs | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | not available | ||||
Proposed branch: | lp:~jtv/launchpad/bug-436426-2 | ||||
Merge into: | lp:launchpad | ||||
Diff against target: |
814 lines 9 files modified
configs/development/launchpad-lazr.conf (+4/-0) configs/testrunner/launchpad-lazr.conf (+4/-0) cronscripts/rosetta-poimport.py (+4/-26) lib/canonical/config/schema-lazr.conf (+7/-1) lib/canonical/launchpad/javascript/translations/translations.js (+4/-2) lib/lp/translations/doc/poimport.txt (+20/-9) lib/lp/translations/doc/translationimportqueue.txt (+60/-33) lib/lp/translations/scripts/po_import.py (+161/-160) lib/lp/translations/scripts/tests/test_translations_import.py (+110/-0) |
||||
To merge this branch: | bzr merge lp:~jtv/launchpad/bug-436426-2 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Jeroen T. Vermeulen (community) | Abstain | ||
Edwin Grubbs (community) | code | Approve | |
Review via email: mp+12610@code.launchpad.net |
Commit message
Description of the change
Jeroen T. Vermeulen (jtv) wrote : | # |
Edwin Grubbs (edwin-grubbs) wrote : | # |
Hi Jeroen,
This looks like a nice refactoring. I only have very minor suggestions.
merge-conditional
-Edwin
>=== modified file 'lib/lp/
>--- lib/lp/
>+++ lib/lp/
>@@ -4,6 +4,14 @@
> into Rosetta.
>
> >>> from zope.security.proxy import removeSecurityProxy
>+ >>> from canonical.
>+ >>> from lp.translations
>+ ... TranslationImpo
>+
>+ >>> def clear_queue(queue):
>+ ... """Remove all entries off the import queue."""
>+ ... store = IStore(
>+ ... store.find(
>
>
> == getGuessedPOFile ==
>@@ -1296,6 +1304,15 @@
> If such bad requests do end up on the import queue, the import queue code will
> raise errors about them.
>
>+ >>> def print_import_
>+ ... """List failures recorded in import script instance."""
Should that be singular with an article:
"in an/the import script instance"
or plural without an article:
"in import script instances"
>+ ... for reason, entries in script.
>+ ... print reason
>+ ... for entry in entries:
>+ ... print "-> " + entry
>+
>+ >>> clear_queue(
>+
> >>> entry = translationimpo
> ... u'po/sr.po', 'foo', True, rosetta_experts,
> ... productseries=
>@@ -1306,12 +1323,29 @@
> >>> entry.setStatus
>
> >>> import logging
>- >>> from lp.translations
>- >>> ImportProcess(
>- Traceback (most recent call last):
>- ...
>- AssertionError: Broken translation import queue entry 4 (for evolution):
>- ...
>+ >>> from canonical.
>+ >>> from lp.translations
>+
>+ >>> script = TranslationsImp
>+ >>> script.logger = FakeLogger()
>+ >>> script.main()
>+ DEBUG Starting...
>+ ERROR Entry is approved but has no place to import to.
>+ ...
>+ DEBUG Finished the import process.
>+
>+ >>> print_import_
>+ Entry is approved but has no place to import to.
>+ -> 'po/sr.po' (id ...) in Evolution trunk series
>+
>+The entry is marked as Failed.
>+
>+ >>> print entry.status.name
>+ FAILED
>+
>+This happens for distribution packages as well as products.
>+
>+ >>> clear_queue(
>
> >>> entry = translationimpo
> ... u'po/th.po', 'bar', False, rosetta_experts,
>@@ -1323,25 +1357,21 @@
>
> >>> entry.setStatus
>
>- >>> ImportProcess(
>- Traceback (most recent call last):
>- ...
>- AssertionError: Broken translation import queue entry 4 (for evolution):
>-...
Jeroen T. Vermeulen (jtv) wrote : | # |
Thanks for the review. Your remarks were:
> >=== modified file 'lib/lp/
> >--- lib/lp/
> +0000
> >+++ lib/lp/
> +0000
> >@@ -1296,6 +1304,15 @@
> > If such bad requests do end up on the import queue, the import queue code
> will
> > raise errors about them.
> >
> >+ >>> def print_import_
> >+ ... """List failures recorded in import script instance."""
>
> Should that be singular with an article:
> "in an/the import script instance"
> or plural without an article:
> "in import script instances"
"A." Added.
> >=== modified file 'lib/lp/
> >--- lib/lp/
> >+++ lib/lp/
> >- def run(self):
> >- """Execute the import of entries from the queue."""
> >- # Get the queue.
> >+ # Time goal for one run. It is not exact. The script can run for
> >+ # longer than this, but will know to stop taking on new work.
> >+ # Since the script is run every 9 or 10 minutes, we set the "alarm"
>
>
> I would avoid calling this an alarm, since it makes me wonder if
> you are using signal.alarm(), even though you explain how it really
> works in the preceding sentence.
Okay, I changed it to say goal instead.
> >+ # at 8 minutes. That leaves a bit of time to complete the last
> >+ # ongoing batch of imports.
> >+ time_to_run = timedelta(
> >+
> >+ # Failures to be reported in bulk at closing, so we don't accumulate
> >+ # thousands of oopses for repetitive errors.
> >+ failures = None
>
>
> What is the purpose of initializing it here in addition to in the
> constructor? Is it used to create documentation that knows about
> instance variables?
Yes, just a place to comment it in really. Blame my background in static
languages.
> >+ self._reportFai
> >+
>
>
> Trailing white space.
Fixed. Thanks for spotting it.
Jeroen
Preview Diff
1 | === modified file 'configs/development/launchpad-lazr.conf' |
2 | --- configs/development/launchpad-lazr.conf 2009-09-28 11:12:51 +0000 |
3 | +++ configs/development/launchpad-lazr.conf 2009-10-01 11:13:16 +0000 |
4 | @@ -197,6 +197,10 @@ |
5 | error_dir: /var/tmp/rosettabranches.test |
6 | oops_prefix: RSBR |
7 | |
8 | +[poimport] |
9 | +error_dir: /var/tmp/poimport |
10 | +oops_prefix: POI |
11 | + |
12 | [supermirror_puller] |
13 | error_dir: /var/tmp/codehosting.test |
14 | oops_prefix: SMP |
15 | |
16 | === modified file 'configs/testrunner/launchpad-lazr.conf' |
17 | --- configs/testrunner/launchpad-lazr.conf 2009-08-28 21:02:50 +0000 |
18 | +++ configs/testrunner/launchpad-lazr.conf 2009-10-01 11:13:16 +0000 |
19 | @@ -166,6 +166,10 @@ |
20 | [personalpackagearchive] |
21 | root: /var/tmp/ppa.test/ |
22 | |
23 | +[poimport] |
24 | +error_dir: /var/tmp/poimport.test |
25 | +oops_prefix: TPOI |
26 | + |
27 | [rosettabranches] |
28 | oops_prefix: TRSBR |
29 | error_dir: /var/tmp/rosettabranches.test |
30 | |
31 | === modified file 'cronscripts/rosetta-poimport.py' |
32 | --- cronscripts/rosetta-poimport.py 2009-07-17 00:26:05 +0000 |
33 | +++ cronscripts/rosetta-poimport.py 2009-10-01 11:13:16 +0000 |
34 | @@ -8,31 +8,9 @@ |
35 | import _pythonpath |
36 | |
37 | from canonical.config import config |
38 | -from canonical.database.sqlbase import ISOLATION_LEVEL_READ_COMMITTED |
39 | -from lp.translations.scripts.po_import import ImportProcess |
40 | -from lp.services.scripts.base import LaunchpadCronScript |
41 | - |
42 | -# Time goal for this run. It is not exact. The script will run for longer |
43 | -# than this time, but will know to stop taking on new batches of imports. |
44 | -# Since script is run every 9 or 10 minutes, we set the "alarm" at 8 minutes. |
45 | -# That leaves a bit of time to complete the last ongoing batch of imports. |
46 | -SECONDS_TO_RUN = 8 * 60 |
47 | - |
48 | -class RosettaPOImporter(LaunchpadCronScript): |
49 | - def main(self): |
50 | - self.txn.set_isolation_level(ISOLATION_LEVEL_READ_COMMITTED) |
51 | - process = ImportProcess(self.txn, self.logger, SECONDS_TO_RUN) |
52 | - self.logger.debug('Starting the import process') |
53 | - process.run() |
54 | - self.logger.debug('Finished the import process') |
55 | - |
56 | +from lp.translations.scripts.po_import import TranslationsImport |
57 | |
58 | if __name__ == '__main__': |
59 | - script = RosettaPOImporter('rosetta-poimport', |
60 | - dbuser=config.poimport.dbuser) |
61 | - script.lock_or_quit() |
62 | - try: |
63 | - script.run() |
64 | - finally: |
65 | - script.unlock() |
66 | - |
67 | + script = TranslationsImport( |
68 | + 'rosetta-poimport', dbuser=config.poimport.dbuser) |
69 | + script.lock_and_run() |
70 | |
71 | === modified file 'lib/canonical/config/schema-lazr.conf' |
72 | --- lib/canonical/config/schema-lazr.conf 2009-09-25 08:54:20 +0000 |
73 | +++ lib/canonical/config/schema-lazr.conf 2009-10-01 11:13:16 +0000 |
74 | @@ -1437,6 +1437,13 @@ |
75 | # Set to 'timeout' to make it timeout every time (for tests). |
76 | statement_timeout: 300 |
77 | |
78 | +# See [error_reports]. |
79 | +oops_prefix: none |
80 | +# See [error_reports]. |
81 | +error_dir: none |
82 | +# See [error_reports]. |
83 | +copy_to_zlog: false |
84 | + |
85 | [processmail] |
86 | # The database user which will be used by this process. |
87 | # datatype: string |
88 | @@ -1538,7 +1545,6 @@ |
89 | # See [error_reports]. |
90 | copy_to_zlog: false |
91 | |
92 | - |
93 | [sendbranchmail] |
94 | # The database user which will be used by this process. |
95 | # datatype: string |
96 | |
97 | === modified file 'lib/canonical/launchpad/javascript/translations/translations.js' |
98 | --- lib/canonical/launchpad/javascript/translations/translations.js 2009-09-28 09:14:34 +0000 |
99 | +++ lib/canonical/launchpad/javascript/translations/translations.js 2009-10-01 11:13:16 +0000 |
100 | @@ -118,8 +118,10 @@ |
101 | var spinner_loader = Y.all('#spinner-loader'); |
102 | spinner_loader.set('innerHTML', '<img src="/@@/spinner" alt="" />'); |
103 | var button_markers = Y.all('.show-output'); |
104 | - button_markers.set('innerHTML', compose_button(false)); |
105 | - button_markers.attach('click', show_output); |
106 | + if (button_markers) { |
107 | + button_markers.set('innerHTML', compose_button(false)); |
108 | + button_markers.attach('click', show_output); |
109 | + } |
110 | // Remove the spinner again. |
111 | spinner_loader.set('innerHTML', ''); |
112 | }; |
113 | |
114 | === modified file 'lib/lp/translations/doc/poimport.txt' |
115 | --- lib/lp/translations/doc/poimport.txt 2009-09-21 19:31:07 +0000 |
116 | +++ lib/lp/translations/doc/poimport.txt 2009-10-01 11:13:16 +0000 |
117 | @@ -15,7 +15,7 @@ |
118 | ... ITranslationImportQueue, RosettaImportStatus) |
119 | >>> from lp.registry.model.sourcepackagename import SourcePackageName |
120 | >>> from lp.translations.model.potemplate import POTemplateSubset |
121 | - >>> from lp.translations.scripts.po_import import ImportProcess |
122 | + >>> from lp.translations.scripts.po_import import TranslationsImport |
123 | >>> from lp.translations.scripts.import_queue_gardener import ( |
124 | ... ImportQueueGardener) |
125 | >>> import datetime |
126 | @@ -644,12 +644,15 @@ |
127 | |
128 | >>> import email |
129 | >>> from lp.services.mail import stub |
130 | - >>> process = ImportProcess(transaction, FakeLogger()) |
131 | - >>> process.run() |
132 | + >>> process = TranslationsImport('poimport', test_args=[]) |
133 | + >>> process.logger = FakeLogger() |
134 | + >>> process.main() |
135 | + DEBUG Starting the import process. |
136 | INFO Importing: Template "firefox" in Mozilla Firefox trunk |
137 | INFO Importing: Welsh (cy) ... of firefox in Mozilla Firefox trunk |
138 | INFO Importing: Welsh (cy) ... of firefox in Mozilla Firefox trunk |
139 | INFO Import requests completed. |
140 | + DEBUG Finished the import process. |
141 | |
142 | The import script also generates an email similar to the ones we saw |
143 | composed before, but also sends it. |
144 | @@ -782,11 +785,14 @@ |
145 | Now that these submissions have been approved, the next run of the |
146 | import script picks them up and processes them. |
147 | |
148 | - >>> process = ImportProcess(transaction, FakeLogger()) |
149 | - >>> process.run() |
150 | + >>> process = TranslationsImport('poimport', test_args=[]) |
151 | + >>> process.logger = FakeLogger() |
152 | + >>> process.main() |
153 | + DEBUG Starting the import process. |
154 | INFO Importing: Welsh (cy) ... of firefox in Mozilla Firefox trunk |
155 | INFO Importing: Welsh (cy) ... of firefox in Mozilla Firefox trunk |
156 | INFO Import requests completed. |
157 | + DEBUG Finished the import process. |
158 | |
159 | >>> print entry.status.name |
160 | IMPORTED |
161 | @@ -866,8 +872,10 @@ |
162 | queue, but doesn't become a candidate for processing until warty |
163 | imports are resumed. |
164 | |
165 | - >>> process = ImportProcess(transaction, FakeLogger()) |
166 | - >>> process.run() |
167 | + >>> process = TranslationsImport('poimport', test_args=[]) |
168 | + >>> process.logger = FakeLogger() |
169 | + >>> process.main() |
170 | + DEBUG Starting the import process. |
171 | INFO No requests pending. |
172 | |
173 | >>> print entry.status.name |
174 | @@ -1039,10 +1047,13 @@ |
175 | this, but is unable to. This is unfortunate, but does not faze the |
176 | importer. It completes normally. |
177 | |
178 | - >>> process = ImportProcess(transaction, FakeLogger()) |
179 | - >>> process.run() |
180 | + >>> process = TranslationsImport('poimport', test_args=[]) |
181 | + >>> process.logger = FakeLogger() |
182 | + >>> process.main() |
183 | + DEBUG Starting the import process. |
184 | INFO Importing: Lao ... |
185 | INFO Import requests completed. |
186 | + DEBUG Finished the import process. |
187 | |
188 | >>> print entry.status.name |
189 | FAILED |
190 | |
191 | === modified file 'lib/lp/translations/doc/translationimportqueue.txt' |
192 | --- lib/lp/translations/doc/translationimportqueue.txt 2009-09-25 15:40:58 +0000 |
193 | +++ lib/lp/translations/doc/translationimportqueue.txt 2009-10-01 11:13:16 +0000 |
194 | @@ -4,6 +4,14 @@ |
195 | into Rosetta. |
196 | |
197 | >>> from zope.security.proxy import removeSecurityProxy |
198 | + >>> from canonical.launchpad.interfaces.lpstorm import IStore |
199 | + >>> from lp.translations.model.translationimportqueue import ( |
200 | + ... TranslationImportQueueEntry) |
201 | + |
202 | + >>> def clear_queue(queue): |
203 | + ... """Remove all entries off the import queue.""" |
204 | + ... store = IStore(TranslationImportQueueEntry) |
205 | + ... store.find(TranslationImportQueueEntry).remove() |
206 | |
207 | |
208 | == getGuessedPOFile == |
209 | @@ -1296,6 +1304,15 @@ |
210 | If such bad requests do end up on the import queue, the import queue code will |
211 | raise errors about them. |
212 | |
213 | + >>> def print_import_failures(import_script): |
214 | + ... """List failures recorded in an import script instance.""" |
215 | + ... for reason, entries in script.failures.iteritems(): |
216 | + ... print reason |
217 | + ... for entry in entries: |
218 | + ... print "-> " + entry |
219 | + |
220 | + >>> clear_queue(translationimportqueue) |
221 | + |
222 | >>> entry = translationimportqueue.addOrUpdateEntry( |
223 | ... u'po/sr.po', 'foo', True, rosetta_experts, |
224 | ... productseries=evolution_productseries) |
225 | @@ -1306,12 +1323,29 @@ |
226 | >>> entry.setStatus(RosettaImportStatus.APPROVED) |
227 | |
228 | >>> import logging |
229 | - >>> from lp.translations.scripts.po_import import ImportProcess |
230 | - >>> ImportProcess(transaction, logging).run() |
231 | - Traceback (most recent call last): |
232 | - ... |
233 | - AssertionError: Broken translation import queue entry 4 (for evolution): |
234 | - ... |
235 | + >>> from canonical.launchpad.scripts import FakeLogger |
236 | + >>> from lp.translations.scripts.po_import import TranslationsImport |
237 | + |
238 | + >>> script = TranslationsImport('poimport', test_args=[]) |
239 | + >>> script.logger = FakeLogger() |
240 | + >>> script.main() |
241 | + DEBUG Starting... |
242 | + ERROR Entry is approved but has no place to import to. |
243 | + ... |
244 | + DEBUG Finished the import process. |
245 | + |
246 | + >>> print_import_failures(script) |
247 | + Entry is approved but has no place to import to. |
248 | + -> 'po/sr.po' (id ...) in Evolution trunk series |
249 | + |
250 | +The entry is marked as Failed. |
251 | + |
252 | + >>> print entry.status.name |
253 | + FAILED |
254 | + |
255 | +This happens for distribution packages as well as products. |
256 | + |
257 | + >>> clear_queue(translationimportqueue) |
258 | |
259 | >>> entry = translationimportqueue.addOrUpdateEntry( |
260 | ... u'po/th.po', 'bar', False, rosetta_experts, |
261 | @@ -1323,25 +1357,21 @@ |
262 | |
263 | >>> entry.setStatus(RosettaImportStatus.APPROVED) |
264 | |
265 | - >>> ImportProcess(transaction, logging).run() |
266 | - Traceback (most recent call last): |
267 | - ... |
268 | - AssertionError: Broken translation import queue entry 4 (for evolution): |
269 | - ... |
270 | + >>> script = TranslationsImport('poimport', test_args=[]) |
271 | + >>> script.logger.setLevel(logging.FATAL) |
272 | + >>> script.main() |
273 | + >>> print_import_failures(script) |
274 | + Entry is approved but has no place to import to. |
275 | + -> 'po/th.po' (id ...) in ubuntu Hoary package evolution |
276 | + |
277 | + >>> print entry.status.name |
278 | + FAILED |
279 | + |
280 | + >>> clear_queue(translationimportqueue) |
281 | |
282 | |
283 | == getRequestTargets output ordering == |
284 | |
285 | -TranslationImportQueue.getRequestTargets first lists distro series |
286 | -(ordered by distro name and series name), followed by products (ordered |
287 | -by name). |
288 | - |
289 | - >>> print_list(get_target_names(RosettaImportStatus.APPROVED)) |
290 | - ubuntu/hoary Hoary |
291 | - evolution Evolution |
292 | - firefox Mozilla Firefox |
293 | - |
294 | - |
295 | The queue is populated with a wild mix of requests: for packages in |
296 | different release series of Ubuntu, for packages in different distros, |
297 | for product series, with different statuses; some were imported from |
298 | @@ -1388,29 +1418,24 @@ |
299 | >>> entry.setStatus(RosettaImportStatus.NEEDS_REVIEW) |
300 | >>> flush_database_updates() |
301 | |
302 | - |
303 | -The targets (distro series or products) that these requests are for are |
304 | -always listed in the same, predictable order. |
305 | +TranslationImportQueue.getRequestTargets first lists distro series |
306 | +(ordered by distro name and series name), followed by products (ordered |
307 | +by name). |
308 | |
309 | >>> print_list(get_target_names()) |
310 | debian/sarge Sarge |
311 | debian/woody Woody |
312 | kubuntu/krunch Krunch |
313 | ubuntu/grumpy Grumpy |
314 | - ubuntu/hoary Hoary |
315 | alsa-utils alsa-utils |
316 | evolution Evolution |
317 | - firefox Mozilla Firefox |
318 | - netapplet NetApplet |
319 | |
320 | >>> for status in RosettaImportStatus.items: |
321 | ... print "%s:" % status.name |
322 | ... print_list(get_target_names(status)) |
323 | APPROVED: |
324 | kubuntu/krunch Krunch |
325 | - ubuntu/hoary Hoary |
326 | evolution Evolution |
327 | - firefox Mozilla Firefox |
328 | IMPORTED: |
329 | evolution Evolution |
330 | DELETED: |
331 | @@ -1419,13 +1444,15 @@ |
332 | NEEDS_REVIEW: |
333 | debian/sarge Sarge |
334 | debian/woody Woody |
335 | - ubuntu/hoary Hoary |
336 | - evolution Evolution |
337 | - firefox Mozilla Firefox |
338 | - netapplet NetApplet |
339 | BLOCKED: |
340 | ubuntu/grumpy Grumpy |
341 | |
342 | +You can also select by status. |
343 | + |
344 | + >>> print_list(get_target_names(RosettaImportStatus.APPROVED)) |
345 | + kubuntu/krunch Krunch |
346 | + evolution Evolution |
347 | + |
348 | |
349 | == cleanUpQueue == |
350 | |
351 | |
352 | === modified file 'lib/lp/translations/scripts/po_import.py' |
353 | --- lib/lp/translations/scripts/po_import.py 2009-09-18 07:39:51 +0000 |
354 | +++ lib/lp/translations/scripts/po_import.py 2009-10-01 11:13:16 +0000 |
355 | @@ -10,44 +10,135 @@ |
356 | 'ImportProcess', |
357 | ] |
358 | |
359 | -import time |
360 | +from datetime import datetime, timedelta |
361 | +import sys |
362 | |
363 | +from pytz import UTC |
364 | from zope.component import getUtility |
365 | |
366 | from canonical.config import config |
367 | -from canonical.database.sqlbase import flush_database_updates |
368 | from canonical.launchpad import helpers |
369 | from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities |
370 | from lp.translations.interfaces.translationimportqueue import ( |
371 | ITranslationImportQueue, RosettaImportStatus) |
372 | from canonical.launchpad.mail import simple_sendmail |
373 | from canonical.launchpad.mailnotification import MailWrapper |
374 | - |
375 | - |
376 | -class ImportProcess: |
377 | +from canonical.launchpad.webapp import errorlog |
378 | +from lp.services.scripts.base import LaunchpadCronScript |
379 | + |
380 | + |
381 | +class TranslationsImport(LaunchpadCronScript): |
382 | """Import .po and .pot files attached to Rosetta.""" |
383 | - |
384 | - def __init__(self, ztm, logger, max_seconds=3600): |
385 | - """Initialize the ImportProcess object. |
386 | - |
387 | - Arguments: |
388 | - |
389 | - :param ztm: transaction manager to commit our individual imports. |
390 | - |
391 | - :param logger: logging object to log informational messages and errors |
392 | - to. |
393 | - |
394 | - :param max_seconds: "alarm clock": after this many seconds, the job |
395 | - should finish up even if there is more work for it to do. This is a |
396 | - mere guideline; actual processing time may be longer. |
397 | - """ |
398 | - self.ztm = ztm |
399 | - self.logger = logger |
400 | - self.deadline = time.time() + max_seconds |
401 | - |
402 | - def run(self): |
403 | - """Execute the import of entries from the queue.""" |
404 | - # Get the queue. |
405 | + # Time goal for one run. It is not exact. The script can run for |
406 | + # longer than this, but will know to stop taking on new work. |
407 | + # Since the script is run every 9 or 10 minutes, we set the goal |
408 | + # at 8 minutes. That leaves a bit of time to complete the last |
409 | + # ongoing batch of imports. |
410 | + time_to_run = timedelta(minutes=8) |
411 | + |
412 | + # Failures to be reported in bulk at closing, so we don't accumulate |
413 | + # thousands of oopses for repetitive errors. |
414 | + failures = None |
415 | + |
416 | + def __init__(self, *args, **kwargs): |
417 | + super(TranslationsImport, self).__init__(*args, **kwargs) |
418 | + self.failures = {} |
419 | + |
420 | + def _describeEntry(self, entry): |
421 | + """Identify `entry` in a human-readable way.""" |
422 | + if entry.import_into: |
423 | + return "%s (id %d)" % (entry.import_into.title, entry.id) |
424 | + |
425 | + if entry.sourcepackagename: |
426 | + return "'%s' (id %d) in %s %s package %s" % ( |
427 | + entry.path, entry.id, |
428 | + entry.distroseries.distribution.name, |
429 | + entry.distroseries.displayname, |
430 | + entry.sourcepackagename.name) |
431 | + else: |
432 | + return "'%s' (id %d) in %s" % ( |
433 | + entry.path, entry.id, entry.productseries.title) |
434 | + |
435 | + def _reportOops(self, reason, entries, exc_info=None): |
436 | + """Register an oops.""" |
437 | + if exc_info is None: |
438 | + exc_info = sys.exc_info() |
439 | + description = [ |
440 | + ('Reason', reason), |
441 | + ('Queue entries', entries), |
442 | + ] |
443 | + errorlog.globalErrorUtility.raising( |
444 | + exc_info, errorlog.ScriptRequest(description)) |
445 | + |
446 | + def _registerFailure(self, entry, reason, traceback=False, abort=False): |
447 | + """Note that a queue entry is unusable in some way.""" |
448 | + entry.setStatus(RosettaImportStatus.FAILED) |
449 | + entry.setErrorOutput(reason) |
450 | + |
451 | + if abort: |
452 | + traceback = True |
453 | + |
454 | + description = self._describeEntry(entry) |
455 | + message = "%s -- %s" % (reason, description) |
456 | + self.logger.error(message, exc_info=traceback) |
457 | + |
458 | + if abort: |
459 | + # Serious enough to stop the script. Register as an |
460 | + # individual oops. |
461 | + self._reportOops(self, reason, [description]) |
462 | + else: |
463 | + # Register problem for bulk reporting later. |
464 | + if not self.failures.get(reason): |
465 | + self.failures[reason] = [] |
466 | + self.failures[reason].append(description) |
467 | + |
468 | + def _checkEntry(self, entry): |
469 | + """Sanity-check `entry` before importing.""" |
470 | + if entry.import_into is None: |
471 | + self._registerFailure( |
472 | + entry, "Entry is approved but has no place to import to.") |
473 | + return False |
474 | + |
475 | + template = entry.potemplate |
476 | + if template: |
477 | + if template.distroseries != entry.distroseries: |
478 | + self._registerFailure( |
479 | + entry, "Entry was approved for the wrong distroseries.") |
480 | + return False |
481 | + if template.productseries != entry.productseries: |
482 | + self._registerFailure( |
483 | + entry, "Entry was approved for the wrong productseries.") |
484 | + return False |
485 | + |
486 | + return True |
487 | + |
488 | + def _importEntry(self, entry): |
489 | + """Perform the import of one entry, and notify the uploader.""" |
490 | + self.logger.info('Importing: %s' % entry.import_into.title) |
491 | + target = entry.import_into |
492 | + (mail_subject, mail_body) = target.importFromQueue(entry, self.logger) |
493 | + |
494 | + if mail_subject is not None: |
495 | + # A `mail_subject` of None indicates that there |
496 | + # is no notification worth sending out. |
497 | + from_email = config.rosetta.admin_email |
498 | + katie = getUtility(ILaunchpadCelebrities).katie |
499 | + if entry.importer == katie: |
500 | + # Email import state to Debian imports email. |
501 | + to_email = config.rosetta.debian_import_email |
502 | + else: |
503 | + to_email = helpers.get_contact_email_addresses(entry.importer) |
504 | + |
505 | + if to_email: |
506 | + text = MailWrapper().format(mail_body) |
507 | + simple_sendmail(from_email, to_email, mail_subject, text) |
508 | + |
509 | + def main(self): |
510 | + """Import entries from the queue.""" |
511 | + self.logger.debug("Starting the import process.") |
512 | + |
513 | + errorlog.globalErrorUtility.configure('poimport') |
514 | + self.deadline = datetime.now(UTC) + self.time_to_run |
515 | translation_import_queue = getUtility(ITranslationImportQueue) |
516 | |
517 | # Get the list of each product or distroseries with pending imports. |
518 | @@ -60,138 +151,48 @@ |
519 | self.logger.info("No requests pending.") |
520 | return |
521 | |
522 | - # XXX: JeroenVermeulen 2007-06-20: How on Earth do we test that the |
523 | - # deadline code works? It's only a small thing, and of course we'll |
524 | - # notice that it works when we stop getting errors about this script |
525 | - # not finishing. Meanwhile, SteveA has suggested a more general |
526 | - # solution. |
527 | - while importqueues and time.time() < self.deadline: |
528 | - # For fairness, service all queues at least once; don't check for |
529 | - # deadline. If we stopped halfway through the list of queues, we |
530 | - # would accidentally favour queues that happened to come out at |
531 | - # the front of the list. |
532 | + have_work = True |
533 | + |
534 | + while have_work and datetime.now(UTC) < self.deadline: |
535 | + have_work = False |
536 | + |
537 | + # For fairness, service all queues at least once; don't |
538 | + # check for deadlines here or we'd favour some |
539 | + # products/packages over others. |
540 | for queue in importqueues: |
541 | - # Make sure our previous state changes hit the database. |
542 | - # Otherwise, getFirstEntryToImport() might pick up an entry |
543 | - # we've already processed but haven't flushed yet. |
544 | - # XXX: JeroenVermeulen 2007-11-29 bug=3989: should become |
545 | - # unnecessary once Zopeless commit() improves. |
546 | - flush_database_updates() |
547 | - |
548 | - entry_to_import = queue.getFirstEntryToImport() |
549 | - if entry_to_import is None: |
550 | - continue |
551 | - |
552 | - if entry_to_import.import_into is None: |
553 | - if entry_to_import.sourcepackagename is not None: |
554 | - package = entry_to_import.sourcepackagename.name |
555 | - elif entry_to_import.productseries is not None: |
556 | - package = ( |
557 | - entry_to_import.productseries.product.displayname) |
558 | - else: |
559 | - raise AssertionError( |
560 | - "Import queue entry %d has neither a " |
561 | - "source package name nor a product series." |
562 | - % entry_to_import.id) |
563 | - raise AssertionError( |
564 | - "Broken translation import queue entry %d (for %s): " |
565 | - "it's Approved but lacks the place where it should " |
566 | - "be imported! A DBA will need to fix this by hand." |
567 | - % (entry_to_import.id, package)) |
568 | - |
569 | - # Do the import. |
570 | - title = '[Unknown Title]' |
571 | - try: |
572 | - title = entry_to_import.import_into.title |
573 | - self.logger.info('Importing: %s' % title) |
574 | - |
575 | - (mail_subject, mail_body) = ( |
576 | - entry_to_import.import_into.importFromQueue( |
577 | - entry_to_import, self.logger)) |
578 | - |
579 | - if mail_subject is not None: |
580 | - # A `mail_subject` of None indicates that there |
581 | - # is no notification worth sending out. |
582 | - from_email = config.rosetta.admin_email |
583 | - katie = getUtility(ILaunchpadCelebrities).katie |
584 | - if entry_to_import.importer == katie: |
585 | - # Email import state to Debian imports email. |
586 | - to_email = config.rosetta.debian_import_email |
587 | - else: |
588 | - to_email = helpers.get_contact_email_addresses( |
589 | - entry_to_import.importer) |
590 | - |
591 | - if to_email: |
592 | - text = MailWrapper().format(mail_body) |
593 | - |
594 | - # XXX: JeroenVermeulen 2007-11-29 bug=29744: email |
595 | - # isn't transactional in zopeless mode. That |
596 | - # means that our current transaction can fail |
597 | - # after we already sent out a success |
598 | - # notification. To prevent that, we commit the |
599 | - # import (attempt) before sending out the email. |
600 | - # That way the worst that can happen is that an |
601 | - # email goes missing. |
602 | - # Once bug 29744 is fixed, this commit must die so |
603 | - # the email and the import will be in a single |
604 | - # atomic operation. |
605 | - self.ztm.commit() |
606 | - self.ztm.begin() |
607 | - |
608 | - simple_sendmail( |
609 | - from_email, to_email, mail_subject, text) |
610 | - |
611 | - except KeyboardInterrupt: |
612 | - self.ztm.abort() |
613 | - raise |
614 | - except AssertionError: |
615 | - raise |
616 | - except: |
617 | - # If we have any exception, log it, abort the transaction |
618 | - # and set the status to FAILED. |
619 | - self.logger.error('Got an unexpected exception while' |
620 | - ' importing %s' % title, exc_info=1) |
621 | - # We are going to abort the transaction, need to save the |
622 | - # id of this entry to update its status. |
623 | - failed_entry_id = entry_to_import.id |
624 | - self.ztm.abort() |
625 | - # Get the needed objects to set the failed entry status as |
626 | - # FAILED. |
627 | - self.ztm.begin() |
628 | - translation_import_queue = getUtility( |
629 | - ITranslationImportQueue) |
630 | - entry_to_import = translation_import_queue[ |
631 | - failed_entry_id] |
632 | - entry_to_import.setStatus(RosettaImportStatus.FAILED) |
633 | - self.ztm.commit() |
634 | - self.ztm.begin() |
635 | - # Go to process next entry. |
636 | - continue |
637 | - |
638 | - # As soon as the import is done, we commit the transaction |
639 | - # so it's not lost. |
640 | - try: |
641 | - self.ztm.commit() |
642 | - self.ztm.begin() |
643 | - except KeyboardInterrupt: |
644 | - self.ztm.abort() |
645 | - raise |
646 | - except: |
647 | - # If we have any exception, we log it and abort the |
648 | - # transaction. |
649 | - self.logger.error( |
650 | - 'We got an unexpected exception while committing the ' |
651 | - 'transaction', |
652 | - exc_info=1) |
653 | - self.ztm.abort() |
654 | - self.ztm.begin() |
655 | - |
656 | - # Refresh the list of objects with pending imports. |
657 | - importqueues = ( |
658 | - translation_import_queue.getRequestTargets( |
659 | - RosettaImportStatus.APPROVED)) |
660 | - |
661 | - if not importqueues: |
662 | + entry = queue.getFirstEntryToImport() |
663 | + if entry is None: |
664 | + continue |
665 | + |
666 | + have_work = True |
667 | + |
668 | + try: |
669 | + if self._checkEntry(entry): |
670 | + self._importEntry(entry) |
671 | + if self.txn: |
672 | + self.txn.commit() |
673 | + except KeyboardInterrupt: |
674 | + raise |
675 | + except (AssertionError, SystemError), e: |
676 | + self._registerFailure(entry, e, abort=True) |
677 | + raise |
678 | + except Exception, e: |
679 | + if self.txn: |
680 | + self.txn.abort() |
681 | + self._registerFailure(entry, e, traceback=True) |
682 | + if self.txn: |
683 | + self.txn.commit() |
684 | + |
685 | + if have_work: |
686 | + self.logger.info("Used up available time.") |
687 | + else: |
688 | self.logger.info("Import requests completed.") |
689 | - else: |
690 | - self.logger.info("Used up available time.") |
691 | + |
692 | + self._reportFailures() |
693 | + |
694 | + self.logger.debug("Finished the import process.") |
695 | + |
696 | + def _reportFailures(self): |
697 | + """Bulk-report deferred failures as oopses.""" |
698 | + for reason, entries in self.failures.iteritems(): |
699 | + self._reportOops(reason, entries) |
700 | |
701 | === added file 'lib/lp/translations/scripts/tests/test_translations_import.py' |
702 | --- lib/lp/translations/scripts/tests/test_translations_import.py 1970-01-01 00:00:00 +0000 |
703 | +++ lib/lp/translations/scripts/tests/test_translations_import.py 2009-10-01 11:13:16 +0000 |
704 | @@ -0,0 +1,110 @@ |
705 | +# Copyright 2009 Canonical Ltd. This software is licensed under the |
706 | +# GNU Affero General Public License version 3 (see the file LICENSE). |
707 | + |
708 | +import logging |
709 | +import re |
710 | + |
711 | +from unittest import TestLoader |
712 | + |
713 | +from lp.testing import TestCaseWithFactory |
714 | +from canonical.testing.layers import LaunchpadScriptLayer |
715 | + |
716 | +from lp.translations.model.translationimportqueue import ( |
717 | + TranslationImportQueue) |
718 | +from lp.translations.scripts.po_import import TranslationsImport |
719 | + |
720 | + |
721 | +class TestTranslationsImport(TestCaseWithFactory): |
722 | + |
723 | + layer = LaunchpadScriptLayer |
724 | + |
725 | + def setUp(self): |
726 | + super(TestTranslationsImport, self).setUp() |
727 | + self.queue = TranslationImportQueue() |
728 | + self.script = TranslationsImport('poimport', test_args=[]) |
729 | + self.script.logger.setLevel(logging.FATAL) |
730 | + self.owner = self.factory.makePerson() |
731 | + |
732 | + def _makeProductSeries(self): |
733 | + """Make a product series called 'trunk'.""" |
734 | + return self.factory.makeProduct(owner=self.owner).getSeries('trunk') |
735 | + |
736 | + def _makeEntry(self, path, **kwargs): |
737 | + """Produce a queue entry.""" |
738 | + return self.queue.addOrUpdateEntry( |
739 | + path, '# Nothing here', False, self.owner, **kwargs) |
740 | + |
741 | + def test_describeEntry_without_target(self): |
742 | + productseries = self._makeProductSeries() |
743 | + entry = self._makeEntry('foo.po', productseries=productseries) |
744 | + description = self.script._describeEntry(entry) |
745 | + pattern = "'foo.po' \(id [0-9]+\) in [A-Za-z0-9_-]+ trunk series$" |
746 | + self.assertNotEqual(None, re.match(pattern, description)) |
747 | + |
748 | + def test_describeEntry_for_pofile(self): |
749 | + productseries = self._makeProductSeries() |
750 | + template = self.factory.makePOTemplate(productseries=productseries) |
751 | + pofile = template.newPOFile('nl') |
752 | + entry = self._makeEntry( |
753 | + 'foo.po', productseries=productseries, potemplate=template, |
754 | + pofile=pofile) |
755 | + description = self.script._describeEntry(entry) |
756 | + pattern = "Dutch \(nl\) translation of .* in .* trunk \(id [0-9]+\)$" |
757 | + self.assertNotEqual(None, re.match(pattern, description)) |
758 | + |
759 | + def test_describeEntry_for_template(self): |
760 | + productseries = self._makeProductSeries() |
761 | + template = self.factory.makePOTemplate(productseries=productseries) |
762 | + entry = self._makeEntry( |
763 | + 'foo.pot', productseries=productseries, potemplate=template) |
764 | + description = self.script._describeEntry(entry) |
765 | + pattern = 'Template "[^"]+" in [A-Za-z0-9_-]+ trunk \(id [0-9]+\)$' |
766 | + self.assertNotEqual(None, re.match(pattern, description)) |
767 | + |
768 | + def test_checkEntry(self): |
769 | + productseries = self._makeProductSeries() |
770 | + template = self.factory.makePOTemplate(productseries=productseries) |
771 | + entry = self._makeEntry( |
772 | + 'foo.pot', productseries=productseries, potemplate=template) |
773 | + self.assertTrue(self.script._checkEntry(entry)) |
774 | + |
775 | + def test_checkEntry_without_target(self): |
776 | + productseries = self._makeProductSeries() |
777 | + entry = self._makeEntry('foo.pot', productseries=productseries) |
778 | + self.assertFalse(self.script._checkEntry(entry)) |
779 | + self.assertIn( |
780 | + "Entry is approved but has no place to import to.", |
781 | + self.script.failures.keys()) |
782 | + |
783 | + def test_checkEntry_misapproved_product(self): |
784 | + productseries = self._makeProductSeries() |
785 | + template = self.factory.makePOTemplate() |
786 | + entry = self._makeEntry( |
787 | + 'foo.pot', productseries=productseries, potemplate=template) |
788 | + self.assertNotEqual(None, entry.import_into) |
789 | + |
790 | + self.assertFalse(self.script._checkEntry(entry)) |
791 | + self.assertIn( |
792 | + "Entry was approved for the wrong productseries.", |
793 | + self.script.failures.keys()) |
794 | + |
795 | + def test_checkEntry_misapproved_package(self): |
796 | + package = self.factory.makeSourcePackage() |
797 | + other_series = self.factory.makeDistroRelease( |
798 | + distribution=package.distroseries.distribution) |
799 | + template = self.factory.makePOTemplate( |
800 | + distroseries=package.distroseries, |
801 | + sourcepackagename=package.sourcepackagename) |
802 | + entry = self._makeEntry( |
803 | + 'foo.pot', sourcepackagename=package.sourcepackagename, |
804 | + distroseries=other_series, potemplate=template) |
805 | + self.assertNotEqual(None, entry.import_into) |
806 | + |
807 | + self.assertFalse(self.script._checkEntry(entry)) |
808 | + self.assertIn( |
809 | + "Entry was approved for the wrong distroseries.", |
810 | + self.script.failures.keys()) |
811 | + |
812 | + |
813 | +def test_suite(): |
814 | + return TestLoader().loadTestsFromName(__name__) |
= Bug 436426 =
Bug 436426 says the translations importer should check approved queue
entries for certain inconsistencies. It would have spared us some
serious trouble if we'd done this before the 3.0 rollout.
With apologies, a branch squeezing only just under the review limit at
798 lines. It combines a bunch of improvements to the translations
importer with the actual bug fix:
* Enables oops logging.
* Makes the script class inherit straight from LaunchpadCronSc ript.
* Refactors the script body to be much clearer.
* Replaces assertions with more appropriate error handling.
* Reports potentially repetitive errors in bulk.
* Stops re-fetching the import targets list for every iteration.
* Carries less state along in the doctest by clearing the queue.
A fair amount of diff is actually deletions from code that has moved
into better places. None of the changes in itself is particularly big;
it's just a big pile of cleanups that we'd never have gotten to without
an excuse.
== Detailed description ==
In configs, some configuration is needed for the oops logging. We'll
have to see that this actually works in production environments; recent
experience with the Job infrastructure shows that oops logging can go
horribly wrong if the configuration isn't right, and with no way to
alert developers, nor a way to test since the tests run on a separate
configuration.
In cronscripts: The script class in lp.translations .scripts, renamed orter, now inherits directly from ript. This removes an extra layer of inheritance in the
from ImportProcess to TranslationsImp
LaunchpadCronSc
main script.
In doc/poimport.txt: the test runs now fire off LaunchpadCronSc ripts, so
the debug messages it logs on startup and shutdown show up in the test
output.
In doc/translation importqueue. txt, you'll see how shamelessly we carried
state along the doctest. Made it almost impossible to change anything
except at the very end: every part of the test relied on the byproducts
of everything else before it. I've added a function to clear the queue.
Some errors that used to be reported as assertion failures are now kept
in a "failures" dict inside the script, and reported as oopses at the
end. They are also reported instantly, but for oops reporting, they are
bundled by error message. The idea is that an error affecting 4,600
entries gets oopsed once per script run, but with full details--as
opposed to producing 4,600 individual oopses.
Now we get to the actual script class. The time goal is no longer a
number of seconds living in a global variable; since everything from the
main script has moved in here, it's now a timedelta living in a member.
The failures are aggregated in a dict mapping a failure reason to lists
of queue entries that failed for that reason. That dict is initialized
not in main but in a constructor. This is done so that the unit tests
can exercise it realistically without invoking main. Regular syntax
errors and such are not kept here, but anything that shouldn't be fatal
to the script run yet isn't really expected is.
When you see the identifier "have_work," you're at the place where I
made an unrelated change. The script worked ...