launchpad-reviewers team mailing list archive

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

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #884599 in Launchpad itself: "Translations-to-branch exporter reports locked branch, but not which"
  https://bugs.launchpad.net/launchpad/+bug/884599

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

= Summary =

Failures in the translations-to-branch export script are logged to the error-reports list, but the log entry won't mention the failing branch.  This is a bit of a nuisance because we probably have a persistent failure -- somewhere.

Also, the error message about branches that need to be rescanned is confusing.  Is the branch being skipped because the DB info is out of date while a new scan is scheduled?  Or is the branch being skipped because the DB info is out of date, and is a new scan scheduled to remedy it?  Turns out it's the latter.


== Proposed fix ==

Make the error report name the productseries where export is failing.  Also, disambiguate the error message about out-of-date DB info.


== Pre-implementation notes ==

Didn't have one.  Sorry.  One thing worth noting though is that the error notice mentions not the branch where the failure is encountered, but the productseries.  At least in principle (I doubt it's ever been needed in practice) multiple productseries can export to the same branch, so in that small sense it's more exact.  But more importantly it's easier to find a productseries' translations export branch than it is to find a productseries given the name of its translations export branch.


== Implementation details ==

I also cleaned up some more poor style.


== Tests ==

{{{
./bin/test -vvc lp.translations -t translations-export-to-branch -t test_exportToStaleBranch
}}}


== Demo and Q/A ==

Run on a test codehosting server.  Push a translations export branch for testing.  The test server won't have the other translations export branches, so you'll also see the improved error messages in action.


= Launchpad lint =

There's one piece of lint that I can't avoid.  You'll see why as you read the diff.


Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/scripts/tests/test_translations_to_branch.py
  lib/lp/translations/doc/translations-export-to-branch.txt
  lib/lp/registry/interfaces/productseries.py
  lib/lp/translations/scripts/translations_to_branch.py

./lib/lp/translations/scripts/translations_to_branch.py
      28: 'lp' imported but unused
-- 
https://code.launchpad.net/~jtv/launchpad/bug-884599/+merge/84400
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-884599 into lp:launchpad.

=== modified file 'lib/lp/registry/interfaces/productseries.py'
--- lib/lp/registry/interfaces/productseries.py	2011-05-27 20:03:56 +0000
+++ lib/lp/registry/interfaces/productseries.py	2011-12-04 15:29:32 +0000
@@ -189,9 +189,9 @@
     displayname = exported(
         TextLine(
             title=_('Display Name'),
-            description=_('Display name, in this case we have removed the '
-                          'underlying database field, and this attribute '
-                          'just returns the name.')),
+            description=_(
+                "Display name.  In this case we have removed the underlying "
+                "database field, and this attribute just returns the name.")),
         exported_as='display_name')
 
     releases = exported(

=== modified file 'lib/lp/translations/doc/translations-export-to-branch.txt'
--- lib/lp/translations/doc/translations-export-to-branch.txt	2010-12-29 18:28:58 +0000
+++ lib/lp/translations/doc/translations-export-to-branch.txt	2011-12-04 15:29:32 +0000
@@ -302,7 +302,8 @@
     <BLANKLINE>
     DEBUG ...
     INFO Unlock.
-    ERROR Failure: ConcurrentUpdateError(...Simulated race condition...)
+    ERROR Failure in gazblachko/trunk:
+    ConcurrentUpdateError(...Simulated race condition...)
     INFO Processed 1 item(s); 1 failure(s), 0 unpushed branch(es).
 
 
@@ -322,7 +323,8 @@
     >>> script.main()
     INFO Exporting to translations branches.
     INFO Exporting Gazblachko trunk series.
-    ERROR Failure: ConcurrentUpdateError(...Translations branch for
+    ERROR Failure in gazblachko/trunk:
+    ConcurrentUpdateError(...Translations branch for
     Gazblachko trunk series has pending translations changes.
     Not committing...)
     INFO Processed 1 item(s); 1 failure(s), 0 unpushed branch(es).

=== modified file 'lib/lp/translations/scripts/tests/test_translations_to_branch.py'
--- lib/lp/translations/scripts/tests/test_translations_to_branch.py	2011-10-14 15:15:07 +0000
+++ lib/lp/translations/scripts/tests/test_translations_to_branch.py	2011-12-04 15:29:32 +0000
@@ -169,8 +169,8 @@
             db_branch.last_mirrored_id, tree.branch.last_revision())
         self.assertTrue(db_branch.pending_writes)
         matches = MatchesRegex(
-            '(.|\n)*WARNING Skipped .* due to stale DB info and scheduled'
-            ' scan.')
+            "(.|\n)*WARNING Skipped .* due to stale DB info, and scheduled a "
+            "new scan.")
         self.assertThat(exporter.logger.getLogBuffer(), matches)
 
     def test_exportToBranches_handles_nonascii_exceptions(self):

=== modified file 'lib/lp/translations/scripts/translations_to_branch.py'
--- lib/lp/translations/scripts/translations_to_branch.py	2011-10-14 15:15:07 +0000
+++ lib/lp/translations/scripts/translations_to_branch.py	2011-12-04 15:29:32 +0000
@@ -22,7 +22,9 @@
     )
 from zope.component import getUtility
 
-# Load the normal plugin set. Lint complains but keep this in.
+# Load the normal plugin set.  Your linter may complain, and automated
+# imports formatting tools will rearrange this, but keep it above the
+# other Launchpad imports.
 import lp.codehosting
 
 from canonical.config import config
@@ -184,7 +186,7 @@
             master_branch = IMasterStore(branch).get(Branch, branch.id)
             master_branch.branchChanged(**get_db_branch_info(**e.info))
             self.logger.warning(
-                'Skipped %s due to stale DB info and scheduled scan.',
+                "Skipped %s due to stale DB info, and scheduled a new scan.",
                 branch.bzr_identity)
             if self.txn:
                 self.txn.commit()
@@ -266,7 +268,9 @@
                     self.txn.commit()
             except Exception as e:
                 items_failed += 1
-                self.logger.error("Failure: %s" % repr(e))
+                self.logger.error(
+                    "Failure in %s/%s: %s", source.product.name, source.name,
+                    repr(e))
                 if self.txn:
                     self.txn.abort()