Merge lp:~dobey/tarmac/unapproved-revisions-comment into lp:tarmac

[dobey]

Unify error handling for merge failure or rejection
Add UnaprovedChanges exception for when tip is newer than approved revision

[lp:~dobey/tarmac/unapproved-revisions-comment]

330. By dobey on 2010-08-04

Fix a typo and missing import

[Paul Hummer (rockstar)]

I'm a little unsure about the open "except Exception" but the code indicates that you've thought about that. I'm happy to land this and see if it causes any problems.

review: Approve

=== modified file 'tarmac/bin/commands.py'
--- tarmac/bin/commands.py	2010-08-03 15:38:18 +0000
+++ tarmac/bin/commands.py	2010-08-04 18:58:40 +0000
@@ -5,7 +5,7 @@
 import re
 
 from bzrlib.commands import Command
-from bzrlib.errors import PointlessMerge
+from bzrlib.errors import PointlessMerge, TipChangeRejected
 from bzrlib.help import help_commands
 from launchpadlib.launchpad import (Credentials, Launchpad, EDGE_SERVICE_ROOT,
     STAGING_SERVICE_ROOT)
@@ -15,7 +15,8 @@
 from tarmac.config import TarmacConfig
 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 (BranchHasConflicts, TarmacCommandError,
+                               UnapprovedChanges)
 from tarmac.plugin import load_plugins
 
 
@@ -146,27 +147,65 @@
                     proposal.source_branch, self.config)
 
                 try:
+                    approved = proposal.reviewed_revid
+                    tip = proposal.source_branch.revision_count
+                    if tip > approved:
+                        raise UnapprovedChanges(
+                            'Unapproved changes to branch after approval.')
+
                     self.logger.debug(
                         'Merging %(source)s at revision %(revision)s' % {
                             'source': proposal.source_branch.display_name,
                             'revision': proposal.reviewed_revid,})
+
                     target.merge(
                         source,
                         str(proposal.reviewed_revid))
 
-                except BranchHasConflicts:
-                    subject = (
-                        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 failed '
-                        u'due to merge conflicts:\n\n%(output)s' % {
-                            "source": proposal.source_branch.display_name,
-                            "target": proposal.target_branch.display_name,
-                            "output": target.conflicts})
+                except Exception, 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(BranchHasConflicts, failure):
+                        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(PointlessMerge, failure):
+                        self.logger.warn(
+                            'Merging %(source)s into %(target)s would be '
+                            'pointless.' % {
+                                'source': proposal.source_branch.display_name,
+                                'target': proposal.target_branch.display_name,})
+                        comment = (
+                            u'There is no resulting diff between %(source)s '
+                            u'and %(target)s.' % {
+                                "source": proposal.source_branch.display_name,
+                                "target": proposal.target_branch.display_name,})
+                    elif isinstance(TipChangeRejected, failure):
+                        comment = failure.msg
+                    elif isinstance(UnapprovedChanges, failure):
+                        self.logger.warn(
+                            u'Unapproved chagnes to %(source) were made '
+                            u'after approval for merge into %(target).' % {
+                                "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.')
+                    else:
+                        raise failure
+
                     proposal.createComment(subject=subject, content=comment)
-                    proposal.setStatus(status=u"Needs review")
+                    proposal.setStatus(status=u'Needs review')
                     proposal.lp_save()
                     self.logger.warn(
                         'Conflicts found while merging %(source)s into '
@@ -176,29 +215,6 @@
                     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,})
-
-                    subject = (
-                        u"Pointless merge" % {
-                             "source": proposal.source_branch.display_name,
-                             "target": proposal.target_branch.display_name})
-                    comment = (
-                        u'There is no resulting diff between %(source)s '
-                        u'and %(target)s.' % {
-                            "source": proposal.source_branch.display_name,
-                            "target": proposal.target_branch.display_name,})
-                    proposal.createComment(subject=subject, content=comment)
-                    proposal.setStatus(status=u"Needs review")
-                    proposal.lp_save()
-
-                    target.cleanup()
-                    continue
-
                 urlp = re.compile('http[s]?://api\.(.*)launchpad\.net/beta/')
                 merge_url = urlp.sub(
                     'http://launchpad.net/', proposal.self_link)

=== modified file 'tarmac/exceptions.py'
--- tarmac/exceptions.py	2010-02-12 23:04:28 +0000
+++ tarmac/exceptions.py	2010-08-04 18:58:40 +0000
@@ -29,3 +29,7 @@
 
 class TarmacCommandError(BzrCommandError):
     '''Exception for various command errors.'''
+
+
+class UnapprovedChanges(Exception):
+    '''Exception for when a branch has unapproved changes.'''

=== modified file 'tarmac/plugins/command.py'
--- tarmac/plugins/command.py	2010-07-26 00:06:01 +0000
+++ tarmac/plugins/command.py	2010-08-04 18:58:40 +0000
@@ -65,8 +65,6 @@
 
         if return_code != 0:
             self.do_failed(stdout_value, stderr_value)
-            raise TipChangeRejected(
-                'Test command "%s" failed' % self.verify_command)
 
     def do_failed(self, stdout_value, stderr_value):
         '''Perform failure tests.
@@ -76,6 +74,7 @@
         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' +
                    u'%(output)s') % {
@@ -83,12 +82,7 @@
             'target' : self.proposal.target_branch.display_name,
             'output' : u'\n'.join([stdout_value, stderr_value]),
             }
-        subject = u'Re: [Merge] %s into %s' % (
-            self.proposal.source_branch.display_name,
-            self.proposal.target_branch.display_name)
-        self.proposal.createComment(subject=subject, content=comment)
-        self.proposal.setStatus(status=u'Needs review')
-        self.proposal.lp_save()
+        raise TipChangeRejected(comment)
 
 
 tarmac_hooks['tarmac_pre_commit'].hook(Command(), 'Command plugin')