Merge lp:~dobey/tarmac/commands-errors-decorator into lp:tarmac

[dobey]

Add a new TarmacMergeError exception class for handling merge-preventing errors
Simplify the _do_merges method to use TarmacMergeError
Move comment handling to the point of exception raising
Replace some exceptions with ones derived from TarmacMergeError
Improve some of the exception messages and launchpad comment strings
Document the handling of errors in the writing plug-ins documentation

=== modified file 'docs/writingplugins.txt'
--- docs/writingplugins.txt	2010-07-18 16:01:17 +0000
+++ docs/writingplugins.txt	2010-09-17 22:11:45 +0000
@@ -67,6 +67,51 @@
   The merge proposal that proposes the source branch for merge into the target.
 
 
+
+Handling Errors
+===============
+
+If you are writing a plug-in to validate against some prerequisites for a
+merge to be successful, you may wish to raise an exception if this criteria
+goes unmet, within your plug-in, to prevent the successful merge of a branch.
+The Tarmac merge command automatically handles exceptions. All you need to
+do, is raise the right one. To do this, simply use TarmacMergeError, or
+create your own exception class as a sub-class thereof. For example, if you
+were writing a plug-in to run a test before committing a change, you might
+define your exception like this:
+
+  from tarmac.exceptions import TarmacMergeError
+
+
+  class TestCommandFailed(TarmacMergeError):
+      """The test command failed to complete successfully."""
+
+
+And when you raise your exception to prevent the commit from happening, you
+might do the following:
+
+  message = u'The test command failed to complete successfully.'
+  comment = (u'Tests failed to pass, with the following errors:'
+             u'\n\n%(output)s' % {'output': test_output})
+  raise TestCommandFailed(message, comment)
+
+The TarmacMergeCommand exception takes two arguments. The first is a short
+message summarizing what failed, and the second is a comment that would be
+posted to the merge proposal on Launchpad, to let the developer know what
+needs to be fixed in their branch. The ``message`` argument is similar to
+a normal Python Exception message argument. However, in this case, it should
+be kept to a brief summary of what failed, as the message will be appended
+to another short string for writing to a log. The ``comment`` argument is
+typically a longer message describing the issue in more detail. This comment
+will be posted to the merge proposal on Launchpad, to inform the developer
+of what went wrong during the merge of their branch. The merge proposal will
+also be set back to the ``Needs Review`` status, to avoid having tarmac
+attempt to merge the branch again, without having the issues resolved. If
+no ``comment`` argument is supplied, the ``message`` argument will be used
+for the comment when posting to Launchpad, instead.
+
+
+
 Caveats
 =======
 

=== modified file 'tarmac/bin/commands.py'
--- tarmac/bin/commands.py	2010-09-02 15:18:07 +0000
+++ tarmac/bin/commands.py	2010-09-17 22:11:45 +0000
@@ -5,7 +5,7 @@
 import re
 
 from bzrlib.commands import Command
-from bzrlib.errors import PointlessMerge, TipChangeRejected
+from bzrlib.errors import PointlessMerge
 from bzrlib.help import help_commands
 from launchpadlib.launchpad import (Credentials, Launchpad, EDGE_SERVICE_ROOT,
     STAGING_SERVICE_ROOT)
@@ -14,7 +14,7 @@
 from tarmac.branch import Branch
 from tarmac.hooks import tarmac_hooks
 from tarmac.log import set_up_debug_logging, set_up_logging
-from tarmac.exceptions import (BranchHasConflicts, TarmacCommandError,
+from tarmac.exceptions import (TarmacMergeError, TarmacCommandError,
                                UnapprovedChanges)
 from tarmac.plugin import load_plugins
 
@@ -149,8 +149,12 @@
                     approved = proposal.reviewed_revid
                     tip = proposal.source_branch.revision_count
                     if tip > approved:
-                        raise UnapprovedChanges(
-                            'Unapproved changes to branch after approval.')
+                        message = u'Unapproved changes made after approval'
+                        lp_comment = (
+                            u'There are additional revisions which have not '
+                            u'been approved in review. Please seek review and '
+                            u'approval of these new revisions.')
+                        raise UnapprovedChanges(message, lp_comment)
 
                     self.logger.debug(
                         'Merging %(source)s at revision %(revision)s' % {
@@ -165,50 +169,34 @@
                     tarmac_hooks['tarmac_pre_commit'].fire(
                         self, target, source, proposal)
 
-                except Exception, failure:
+                except TarmacMergeError, failure:
+                    self.logger.warn(
+                        u'Merging %(source)s into %(target)s failed: %s' %
+                        str(failure))
+
                     subject = u'Re: [Merge] %(source)s into %(target)s' % {
                         "source": proposal.source_branch.display_name,
                         "target": proposal.target_branch.display_name}
-                    comment = None
-                    if isinstance(failure, BranchHasConflicts):
-                        self.logger.warn(
-                            u'Conflicts merging %(source)s into %(target)s' % {
-                                "source": proposal.source_branch.display_name,
-                                "target": proposal.target_branch.display_name})
-                        comment = (
-                            u'Attempt to merge %(source)s into %(target)s '
-                            u'failed due to merge conflicts:\n\n%(output)s' % {
-                                "source": proposal.source_branch.display_name,
-                                "target": proposal.target_branch.display_name,
-                                "output": target.conflicts})
-                    elif isinstance(failure, PointlessMerge):
-                        self.logger.warn(
-                            'Merging %(source)s into %(target)s would be '
-                            'pointless.' % {
-                                'source': proposal.source_branch.display_name,
-                                'target': proposal.target_branch.display_name})
-                        target.cleanup()
-                        continue
-                    elif isinstance(failure, TipChangeRejected):
-                        comment = failure.msg
-                    elif isinstance(failure, UnapprovedChanges):
-                        self.logger.warn(
-                            u'Unapproved chagnes to %(source)s were made '
-                            u'after approval for merge into %(target)s.' % {
-                                "source": proposal.source_branch.display_name,
-                                "target": proposal.target_branch.display_name})
-                        comment = (
-                            u'There are additional revisions which have not '
-                            u'been approved in review. Please seek review and '
-                            u'approval of these revisions as well.')
+
+                    if failure.comment:
+                        comment = failure.comment
                     else:
-                        raise
+                        comment = str(failure)
 
-                    proposal.createComment(subject=subject, content=comment)
+                    proposal.createComment(subject=subject,
+                                           content=comment)
                     proposal.setStatus(status=u'Needs review')
                     proposal.lp_save()
                     target.cleanup()
                     continue
+                except PointlessMerge:
+                    self.logger.warn(
+                        'Merging %(source)s into %(target)s would be '
+                        'pointless.' % {
+                            'source': proposal.source_branch.display_name,
+                            'target': proposal.target_branch.display_name})
+                    target.cleanup()
+                    continue
 
                 urlp = re.compile('http[s]?://api\.(.*)launchpad\.net/beta/')
                 merge_url = urlp.sub(

=== modified file 'tarmac/branch.py'
--- tarmac/branch.py	2010-09-02 15:04:52 +0000
+++ tarmac/branch.py	2010-09-17 22:11:45 +0000
@@ -30,7 +30,7 @@
 from bzrlib.workingtree import WorkingTree
 
 from tarmac.config import BranchConfig
-from tarmac.exceptions import BranchHasConflicts
+from tarmac.exceptions import BranchHasConflicts, TarmacMergeError
 
 
 class Branch(object):
@@ -93,7 +93,13 @@
         conflict_list = self.tree.merge_from_branch(
             branch.bzr_branch, to_revision=revid)
         if conflict_list:
-            raise BranchHasConflicts
+            message = u'Conflicts merging branch.'
+            lp_comment = (
+                u'Attempt to merge into %(target)s failed due to conflicts: '
+                u'\n\n%(output)s' % {
+                    'target': self.lp_branch.display_name,
+                    "output": self.conflicts})
+            raise BranchHasConflicts(message, lp_comment)
 
     @property
     def conflicts(self):
@@ -119,8 +125,8 @@
         if reviewers:
             for reviewer in reviewers:
                 if '\n' in reviewer:
-                    raise AssertionError('\\n is not a valid character in a '
-                                         ' reviewer identity')
+                    raise TarmacMergeError('\\n is not a valid character in a '
+                                           'reviewer identity.')
             revprops['reviewers'] = '\n'.join(reviewers)
 
         self.tree.commit(commit_message, committer='Tarmac',

=== modified file 'tarmac/exceptions.py'
--- tarmac/exceptions.py	2010-08-04 18:26:35 +0000
+++ tarmac/exceptions.py	2010-09-17 22:11:45 +0000
@@ -19,7 +19,15 @@
 from bzrlib.errors import BzrCommandError
 
 
-class BranchHasConflicts(Exception):
+class TarmacMergeError(Exception):
+    """An error occurred, preventing the merge of a branch."""
+
+    def __init__(self, message, comment=None, *args, **kwargs):
+        super(TarmacMergeError, self).__init__(message, *args, **kwargs)
+        self.comment = comment
+
+
+class BranchHasConflicts(TarmacMergeError):
     '''Exception for when a branch merge has conflicts.'''
 
 
@@ -31,5 +39,5 @@
     '''Exception for various command errors.'''
 
 
-class UnapprovedChanges(Exception):
+class UnapprovedChanges(TarmacMergeError):
     '''Exception for when a branch has unapproved changes.'''

=== modified file 'tarmac/plugins/command.py'
--- tarmac/plugins/command.py	2010-09-02 15:18:07 +0000
+++ tarmac/plugins/command.py	2010-09-17 22:11:45 +0000
@@ -17,7 +17,6 @@
 '''Tarmac plugin for running tests pre-commit.'''
 
 # Head off lint warnings.
-errors = None
 os = None
 subprocess = None
 
@@ -25,14 +24,17 @@
 lazy_import(globals(), '''
     import os
     import subprocess
-
-    from bzrlib import errors
     ''')
 
+from tarmac.exceptions import TarmacMergeError
 from tarmac.hooks import tarmac_hooks
 from tarmac.plugins import TarmacPlugin
 
 
+class VerifyCommandFailed(TarmacMergeError):
+    """Running the verify_command failed."""
+
+
 class Command(TarmacPlugin):
     '''Tarmac plugin for running a test command.
 
@@ -81,15 +83,15 @@
         doesn't attempt to merge it again without human interaction.  An
         exception is then raised to prevent the commit from happening.
         '''
-        self.logger.warn(u'Test command "%s" failed.' % self.verify_command)
-        comment = (u'The attempt to merge %(source)s into %(target)s failed.' +
-                   u'Below is the output from the failed tests.\n\n' +
+        message = u'Test command "%s" failed.' % self.verify_command
+        comment = (u'The attempt to merge %(source)s into %(target)s failed. '
+                   u'Below is the output from the failed tests.\n\n'
                    u'%(output)s') % {
             'source': self.proposal.source_branch.display_name,
             'target': self.proposal.target_branch.display_name,
             'output': u'\n'.join([stdout_value, stderr_value]),
             }
-        raise errors.TipChangeRejected(comment)
+        raise VerifyCommandFailed(message, comment)
 
 
 tarmac_hooks['tarmac_pre_commit'].hook(Command(), 'Command plugin')

=== modified file 'tarmac/plugins/tests/test_votes.py'
--- tarmac/plugins/tests/test_votes.py	2010-09-09 15:01:57 +0000
+++ tarmac/plugins/tests/test_votes.py	2010-09-17 22:11:45 +0000
@@ -89,7 +89,7 @@
                 ("Voting does not meet specified criteria. "
                  "Required: Approve >= 2, Needs Information == 0. "
                  "Got: 1 Abstain, 2 Approve, 1 Needs Information."),
-                str(error))
+                error.comment)
         else:
             self.fail("Votes.run() did not raise VotingViolation.")
 
@@ -115,6 +115,6 @@
                 ("Voting does not meet specified criteria. "
                  "Required: Approve >= 2, Needs Information == 0. "
                  "Got: 1 Abstain, 2 Approve, 1 Needs Information."),
-                str(error))
+                error.comment)
         else:
             self.fail("Votes.run() did not raise VotingViolation.")

=== modified file 'tarmac/plugins/votes.py'
--- tarmac/plugins/votes.py	2010-09-09 15:01:57 +0000
+++ tarmac/plugins/votes.py	2010-09-17 22:11:45 +0000
@@ -18,6 +18,7 @@
 import operator
 import re
 
+from tarmac.exceptions import TarmacMergeError
 from tarmac.hooks import tarmac_hooks
 from tarmac.plugins import TarmacPlugin
 
@@ -46,7 +47,7 @@
     """A voting criterion is not understood."""
 
 
-class VotingViolation(Exception):
+class VotingViolation(TarmacMergeError):
     """The voting criteria have not been met."""
 
 
@@ -80,9 +81,10 @@
             criteria_desc = ", ".join(
                 "%s %s %d" % (vote, operator_map_inverse[op], value)
                 for (vote, op, value) in criteria)
-            raise VotingViolation(
-                "Voting does not meet specified criteria. "
-                 "Required: %s. Got: %s." % (criteria_desc, votes_desc))
+            lp_comment = (
+                u"Voting does not meet specified criteria. "
+                u"Required: %s. Got: %s." % (criteria_desc, votes_desc))
+            raise VotingViolation(u'Voting criteria not met.', lp_comment)
 
     def count_votes(self, votes):
         """Count and collate the votes.
@@ -108,7 +110,8 @@
             if len(expression) > 0:
                 match = criteria_expr.match(expression)
                 if match is None:
-                    raise InvalidCriterion(expression)
+                    raise InvalidCriterion('Invalid voting criterion: %s' %
+                                           expression)
                 else:
                     vote, op, value = match.groups()
                     op, value = operator_map[op], int(value)