Merge lp:~matsubara/launchpad/ec2-land-update-mp into lp:launchpad

[Diogo Matsubara]

[r=mars][ui=none][bug=672519] Change ec2 land command to update the merge proposal commit message attribute with the built commit message with proper QA tags.

Hi there,

this branch changes the ec2 land command to add the built commit message to the merge proposal. This is done in preparation for https://dev.launchpad.net/Foundations/Proposals/SimplifyMergeMachinery

It also changes the method to get_commit_message to build_commit_message to make it more clear what it's doing.

To run tests: bin/test -tt test_autoland

[Māris Fogels (mars)]

Hi Diogo,

This code looks very good. r=mars.

review: Approve

=== modified file 'lib/devscripts/autoland.py'
--- lib/devscripts/autoland.py	2010-10-24 21:00:11 +0000
+++ lib/devscripts/autoland.py	2010-11-05 20:24:17 +0000
@@ -160,21 +160,35 @@
         # URL. Do it ourselves.
         return URI(scheme='bzr+ssh', host=host, path='/' + branch.unique_name)
 
-    def get_commit_message(self, commit_text, testfix=False, no_qa=False,
+    def build_commit_message(self, commit_text, testfix=False, no_qa=False,
                            incremental=False, rollback=None):
         """Get the Launchpad-style commit message for a merge proposal."""
         reviews = self.get_reviews()
         bugs = self.get_bugs()
 
-        tags = ''.join([
+        tags = [
             get_testfix_clause(testfix),
             get_reviewer_clause(reviews),
             get_bugs_clause(bugs),
             get_qa_clause(bugs, no_qa,
                 incremental, rollback=rollback),
-            ])
-
-        return '%s %s' % (tags, commit_text)
+            ]
+
+        # Make sure we don't add duplicated tags to commit_text.
+        commit_tags = tags[:]
+        for tag in tags:
+            if tag in commit_text:
+                commit_tags.remove(tag)
+
+        if commit_tags:
+            return '%s %s' % (''.join(commit_tags), commit_text)
+        else:
+            return commit_text
+
+    def set_commit_message(self, commit_message):
+        """Set the Launchpad-style commit message for a merge proposal."""
+        self._mp.commit_message = commit_message
+        self._mp.lp_save()
 
 
 def get_testfix_clause(testfix=False):

=== modified file 'lib/devscripts/ec2test/builtins.py'
--- lib/devscripts/ec2test/builtins.py	2010-10-29 14:52:10 +0000
+++ lib/devscripts/ec2test/builtins.py	2010-11-05 20:24:17 +0000
@@ -411,7 +411,7 @@
         if rollback and (no_qa or incremental):
             print "--rollback option used. Ignoring --no-qa and --incremental."
         try:
-            commit_message = mp.get_commit_message(
+            commit_message = mp.build_commit_message(
                 commit_text, testfix, no_qa, incremental, rollback=rollback)
         except MissingReviewError:
             raise BzrCommandError(
@@ -428,6 +428,15 @@
                 "--incremental option requires bugs linked to the branch. "
                 "Link the bugs or remove the --incremental option.")
 
+        # Override the commit message in the MP with the commit message built
+        # with the proper tags.
+        try:
+            mp.set_commit_message(commit_message)
+        except Exception, e:
+            raise BzrCommandError(
+                "Unable to set the commit message in the merge proposal.\n"
+                "Got: %s" % e)
+
         if print_commit:
             print commit_message
             return

=== modified file 'lib/devscripts/tests/test_autoland.py'
--- lib/devscripts/tests/test_autoland.py	2010-09-03 15:13:01 +0000
+++ lib/devscripts/tests/test_autoland.py	2010-11-05 20:24:17 +0000
@@ -59,6 +59,10 @@
 
     def __init__(self, root=None):
         self._root = root
+        self.commit_message = None
+
+    def lp_save(self):
+        pass
 
 
 class TestPQMRegexAcceptance(unittest.TestCase):
@@ -91,7 +95,7 @@
             raise self.failureException(msg)
 
     def _test_commit_message_match(self, incr, no_qa, testfix):
-        commit_message = self.mp.get_commit_message("Foobaring the sbrubble.",
+        commit_message = self.mp.build_commit_message("Foobaring the sbrubble.",
             testfix, no_qa, incr)
         self.assertRegexpMatches(commit_message, self.devel_open_re)
         self.assertRegexpMatches(commit_message, self.dbdevel_normal_re)
@@ -150,7 +154,7 @@
         self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
 
         self.assertEqual("[r=foo][ui=none][bug=20] Foobaring the sbrubble.",
-            self.mp.get_commit_message("Foobaring the sbrubble.",
+            self.mp.build_commit_message("Foobaring the sbrubble.",
                 testfix, no_qa, incr))
 
     def test_commit_no_bugs_no_noqa(self):
@@ -161,7 +165,7 @@
         self.mp.get_bugs = FakeMethod([])
         self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
 
-        self.assertRaises(MissingBugsError, self.mp.get_commit_message,
+        self.assertRaises(MissingBugsError, self.mp.build_commit_message,
             testfix, no_qa, incr)
 
     def test_commit_no_bugs_with_noqa(self):
@@ -173,7 +177,7 @@
         self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
 
         self.assertEqual("[r=foo][ui=none][no-qa] Foobaring the sbrubble.",
-            self.mp.get_commit_message("Foobaring the sbrubble.",
+            self.mp.build_commit_message("Foobaring the sbrubble.",
                 testfix, no_qa, incr))
 
     def test_commit_bugs_with_noqa(self):
@@ -186,7 +190,7 @@
 
         self.assertEqual(
             "[r=foo][ui=none][bug=20][no-qa] Foobaring the sbrubble.",
-            self.mp.get_commit_message("Foobaring the sbrubble.",
+            self.mp.build_commit_message("Foobaring the sbrubble.",
                 testfix, no_qa, incr))
 
     def test_commit_bugs_with_incr(self):
@@ -199,7 +203,7 @@
 
         self.assertEqual(
             "[r=foo][ui=none][bug=20][incr] Foobaring the sbrubble.",
-            self.mp.get_commit_message("Foobaring the sbrubble.",
+            self.mp.build_commit_message("Foobaring the sbrubble.",
                 testfix, no_qa, incr))
 
     def test_commit_no_bugs_with_incr(self):
@@ -212,7 +216,7 @@
 
         self.assertEqual(
             "[r=foo][ui=none][bug=20][incr] Foobaring the sbrubble.",
-            self.mp.get_commit_message("Foobaring the sbrubble.",
+            self.mp.build_commit_message("Foobaring the sbrubble.",
                 testfix, no_qa, incr))
 
     def test_commit_with_noqa_and_incr(self):
@@ -225,7 +229,7 @@
 
         self.assertEqual(
             "[r=foo][ui=none][bug=20][no-qa][incr] Foobaring the sbrubble.",
-            self.mp.get_commit_message("Foobaring the sbrubble.", 
+            self.mp.build_commit_message("Foobaring the sbrubble.", 
                 testfix, no_qa, incr))
 
     def test_commit_with_rollback(self):
@@ -234,8 +238,29 @@
 
         self.assertEqual(
             "[r=foo][ui=none][bug=20][rollback=123] Foobaring the sbrubble.",
-            self.mp.get_commit_message("Foobaring the sbrubble.", 
-                rollback=123))
+            self.mp.build_commit_message("Foobaring the sbrubble.", 
+                rollback=123))
+
+    def test_takes_into_account_existing_tags_on_commit_text(self):
+        self.mp.get_bugs = FakeMethod([self.fake_bug])
+        self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
+
+        self.assertEqual(
+            "[r=foo][ui=none][bug=20][rollback=123] Foobaring the sbrubble.",
+            self.mp.build_commit_message(
+                "[r=foo][ui=none][bug=20][rollback=123] Foobaring the sbrubble.",
+                rollback=123))
+
+
+class TestSetCommitMessage(unittest.TestCase):
+
+    def setUp(self):
+        self.mp = MergeProposal(FakeLPMergeProposal())
+
+    def test_set_commit_message(self):
+        commit_message = "Foobaring the sbrubble."
+        self.mp.set_commit_message(commit_message)
+        self.assertEqual(self.mp._mp.commit_message, commit_message)
 
 
 class TestGetTestfixClause(unittest.TestCase):