launchpad-reviewers team mailing list archive

[Diogo Matsubara <matsubara@xxxxxxxxxxxxx>]

Diogo Matsubara has proposed merging lp:~matsubara/launchpad/ec2-land-update-mp into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)


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
-- 
https://code.launchpad.net/~matsubara/launchpad/ec2-land-update-mp/+merge/40231
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~matsubara/launchpad/ec2-land-update-mp into lp:launchpad/devel.

=== 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):