Bazaar PQM Plugin

Merge lp:~ursinha/bzr-pqm/add-noqa-incr-lpland into lp:bzr-pqm

  1. add-noqa-incr-lpland
  2. Merge into devel
Proposed by Ursula Junque
Status: Merged
Approved by: John A Meinel
Approved revision: 81
Merged at revision: 71
Proposed branch: lp:~ursinha/bzr-pqm/add-noqa-incr-lpland
Merge into: lp:bzr-pqm
Diff against target: 448 lines (+284/-22)
4 files modified
__init__.py (+44/-9)
lpland.py (+61/-12)
tests/fakemethod.py (+21/-0)
tests/test_lpland.py (+158/-1)
To merge this branch: bzr merge lp:~ursinha/bzr-pqm/add-noqa-incr-lpland
Reviewer Review Type Date Requested Status
John A Meinel Approve
Review via email: mp+31703@code.launchpad.net

Description of the change

= Summary =

This branch is a fix for bug 602137, adding to bzr lp-land three options: --testfix, --no-qa and --incr. --no-qa option should add the [no-qa] tag to the commit message, --incremental option should add the [incr] tag, and --testfix, [testfix]. This enforces the QA policy of now allowing fixes to be committed without bugs unless explicitely pointed.

== Proposed fix ==

The implementation is pretty simple. A method checks if the --no-qa, --incremental or --testfix options are given to bzr lp-land, and combined with the bugs found in the mp decides if the merge can continue based on the QA policy:

 * If the mp has no linked bugs and no no-qa tag, ec2 land should fail because without bugs and no-qa tag that commit would be an orphaned one.
 * If the mp has bugs and the no-qa commit tag, that's ok. The bug #s and the no-qa tag will be added to the commit message. This will be properly parsed and handled by qa-tagger script after the branch landed.
 * If the mp has no linked bugs but has the no-qa commit tag, that's ok. The commit will be qa-untestable, therefore not orphaned.
 * If the mp has no linked bugs but has the incr commit tag, ec2 land should fail, because bugs are needed when the fix is incremental.
 * A commit cannot be no-qa and incr at the same time.

== Tests ==

I've tested the newly created methods by adding more tests to test_lpland.py. I'm also testing get_commit_message using a very basic implementation of a fakemethod, that mocks launchpadlib's responses.

bzr selftest pqm

== Demo and Q/A ==

I've tested by running the command in a real merge proposal in Launchpad, using the --dry-run option, to check if the commit message is correctly parsed and tagged by bzr lp-land. I landed my branch that does similar changes to ec2 land (https://code.edge.launchpad.net/~ursinha/launchpad/add-ec2land-rules-orphaned-branches-no-conflicts/+merge/31386) using it.

devel/add-ec2land-rules-orphaned-branches-no-conflicts $ bzr lp-land

Results should be as following:
* Both --incremental and --no-qa at the same time: should error.
* without --no-qa and nor linked bugs: should error.
* with --incremental and no linked bugs: should error.
* with --incremental and linked bugs: should work.
* with --no-qa and no linked bugs: should work.
* with --no-qa and linked bugs: should work.

To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Ursula Junque wrote:
> Ursula Junque has proposed merging lp:~ursinha/bzr-pqm/add-noqa-incr-lpland into lp:bzr-pqm.
>
> Requested reviews:
> Bzr-pqm-devel (bzr-pqm-devel)
> Related bugs:
> #602137 add bzr lp-land support to no-qa and incr QA tags
> https://bugs.launchpad.net/bugs/602137
>
>
> = Summary =
>
> This branch is a fix for bug 602137, adding to bzr lp-land three options: --testfix, --no-qa and --incr. --no-qa option should add the [no-qa] tag to the commit message, --incremental option should add the [incr] tag, and --testfix, [testfix]. This enforces the QA policy of now allowing fixes to be committed without bugs unless explicitely pointed.
>
> == Proposed fix ==
>
> The implementation is pretty simple. A method checks if the --no-qa, --incremental or --testfix options are given to bzr lp-land, and combined with the bugs found in the mp decides if the merge can continue based on the QA policy:
>
> * If the mp has no linked bugs and no no-qa tag, ec2 land should fail because without bugs and no-qa tag that commit would be an orphaned one.
> * If the mp has bugs and the no-qa commit tag, that's ok. The bug #s and the no-qa tag will be added to the commit message. This will be properly parsed and handled by qa-tagger script after the branch landed.
> * If the mp has no linked bugs but has the no-qa commit tag, that's ok. The commit will be qa-untestable, therefore not orphaned.
> * If the mp has no linked bugs but has the incr commit tag, ec2 land should fail, because bugs are needed when the fix is incremental.
> * A commit cannot be no-qa and incr at the same time.

As far as reviewing goes, I'm going to trust that you know the Launchpad
requirements more than I do. As such, I'm not really auditing this part.
I'll just look at the code itself.

Personally, I really don't like this notation:
+__metaclass__ = type

Certainly in bzrlib itself we always do:

class Foo(object):

instead.

I'm certainly inclined to be less strict in bzr-pqm, esp. as I think the
Launchpad standard way of doing it is with __metaclass__. (Also, I see
that test_lpland was using __metaclass__ as well.)

I'm going to just land this, but mostly I'm trusting that you've thought
the logic through. Certainly everything I've seen seems well thought out.

John
=:->
 merge: approve

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkxcPVQACgkQJdeBCYSNAAMDNQCcC6688GqhnisqHAjqVti2q3T/
L6kAnivByx3OiMqv8oDA4sV4ZTgosN0Z
=zgkD
-----END PGP SIGNATURE-----

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file '__init__.py'
--- __init__.py 2010-04-14 10:48:03 +0000
+++ __init__.py 2010-08-03 22:01:20 +0000
@@ -18,6 +18,7 @@
1818
19from bzrlib.commands import Command, register_command19from bzrlib.commands import Command, register_command
20from bzrlib.option import Option20from bzrlib.option import Option
21from bzrlib.errors import BzrCommandError
2122
2223
23version_info = (1, 4, 0, 'dev', 0)24version_info = (1, 4, 0, 'dev', 0)
@@ -83,7 +84,7 @@
8384
84 def run(self, location=None, message=None, public_location=None,85 def run(self, location=None, message=None, public_location=None,
85 dry_run=False, submit_branch=None, ignore_local=False):86 dry_run=False, submit_branch=None, ignore_local=False):
86 from bzrlib import errors, trace, bzrdir87 from bzrlib import trace, bzrdir
87 if __name__ != 'bzrlib.plugins.pqm':88 if __name__ != 'bzrlib.plugins.pqm':
88 trace.warning('The bzr-pqm plugin needs to be called'89 trace.warning('The bzr-pqm plugin needs to be called'
89 ' "bzrlib.plugins.pqm" not "%s"\n'90 ' "bzrlib.plugins.pqm" not "%s"\n'
@@ -103,12 +104,12 @@
103 b.lock_read()104 b.lock_read()
104 self.add_cleanup(b.unlock)105 self.add_cleanup(b.unlock)
105 if relpath and not tree and location != '.':106 if relpath and not tree and location != '.':
106 raise errors.BzrCommandError(107 raise BzrCommandError(
107 'No working tree was found, but we were not given the '108 'No working tree was found, but we were not given the '
108 'exact path to the branch.\n'109 'exact path to the branch.\n'
109 'We found a branch at: %s' % (b.base,))110 'We found a branch at: %s' % (b.base,))
110 if message is None:111 if message is None:
111 raise errors.BzrCommandError(112 raise BzrCommandError(
112 'You must supply a commit message for the pqm to use.')113 'You must supply a commit message for the pqm to use.')
113 submit(b, message=message, dry_run=dry_run,114 submit(b, message=message, dry_run=dry_run,
114 public_location=public_location,115 public_location=public_location,
@@ -125,18 +126,53 @@
125126
126 takes_args = ['location?']127 takes_args = ['location?']
127128
128 takes_options = [Option(129 takes_options = [
129 'dry-run', help='Display the PQM message instead of sending.')]130 Option('dry-run', help='Display the PQM message instead of sending.'),
131 Option(
132 'testfix',
133 help="This is a testfix (tags commit with [testfix])."),
134 Option(
135 'no-qa',
136 help="Does not require QA (tags commit with [no-qa])."),
137 Option(
138 'incremental',
139 help="Incremental to other bug fix (tags commit with [incr])."),
140 ]
130141
131 def run(self, location=None, dry_run=False):142 def run(self, location=None, dry_run=False, testfix=False,
143 no_qa=False, incremental=False):
132 from bzrlib.plugins.pqm.lpland import Submitter144 from bzrlib.plugins.pqm.lpland import Submitter
133 from bzrlib import branch as _mod_branch145 from bzrlib import branch as _mod_branch
146 from bzrlib.plugins.pqm.lpland import (
147 MissingReviewError, MissingBugsError, MissingBugsIncrementalError)
148
149
150 if no_qa and incremental:
151 raise BzrCommandError(
152 "--no-qa and --incremental cannot be given at the same time.")
153
134 branch = _mod_branch.Branch.open_containing('.')[0]154 branch = _mod_branch.Branch.open_containing('.')[0]
135 if dry_run:155 if dry_run:
136 outf = self.outf156 outf = self.outf
137 else:157 else:
138 outf = None158 outf = None
139 submitter = Submitter(branch, location).run(outf)159 try:
160 submitter = Submitter(branch, location, testfix, no_qa, incremental
161 ).run(outf)
162 except MissingReviewError:
163 raise BzrCommandError(
164 "Cannot land branches that haven't got approved code "
165 "reviews. Get an 'Approved' vote so we can fill in the "
166 "[r=REVIEWER] section.")
167 except MissingBugsError:
168 raise BzrCommandError(
169 "Branch doesn't have linked bugs and doesn't have no-qa "
170 "option set. Use --no-qa, or link the related bugs to the "
171 "branch.")
172 except MissingBugsIncrementalError:
173 raise BzrCommandError(
174 "--incremental option requires bugs linked to the branch. "
175 "Link the bugs or remove the --incremental option.")
140176
141177
142register_command(cmd_pqm_submit)178register_command(cmd_pqm_submit)
@@ -147,8 +183,7 @@
147 from bzrlib.tests import TestLoader183 from bzrlib.tests import TestLoader
148 from unittest import TestSuite184 from unittest import TestSuite
149185
150 import test_lpland186 from tests import test_lpland, test_pqm_submit
151 import test_pqm_submit
152187
153 loader = TestLoader()188 loader = TestLoader()
154 return TestSuite([189 return TestSuite([
155190
=== modified file 'lpland.py'
--- lpland.py 2010-04-07 09:47:52 +0000
+++ lpland.py 2010-08-03 22:01:20 +0000
@@ -33,6 +33,14 @@
33 """Raised when we try to get a review message without enough reviewers."""33 """Raised when we try to get a review message without enough reviewers."""
3434
3535
36class MissingBugsError(Exception):
37 """Merge proposal has no linked bugs and no [no-qa] tag."""
38
39
40class MissingBugsIncrementalError(Exception):
41 """Merge proposal has the [incr] tag but no linked bugs."""
42
43
36class LaunchpadBranchLander:44class LaunchpadBranchLander:
3745
38 name = 'launchpad-branch-lander'46 name = 'launchpad-branch-lander'
@@ -171,25 +179,31 @@
171 # XXX: JonathanLange 2009-09-24: No unit tests.179 # XXX: JonathanLange 2009-09-24: No unit tests.
172 return branch.composePublicURL(scheme="bzr+ssh")180 return branch.composePublicURL(scheme="bzr+ssh")
173181
174 def get_commit_message(self, commit_text, testfix=False):182 def get_commit_message(self, commit_text, testfix=False, no_qa=False,
183 incremental=False):
175 """Get the Launchpad-style commit message for a merge proposal."""184 """Get the Launchpad-style commit message for a merge proposal."""
176 reviews = self.get_reviews()185 reviews = self.get_reviews()
177 bugs = self.get_bugs()186 bugs = self.get_bugs()
178 if testfix:187
179 testfix = '[testfix]'188 tags = ''.join([
180 else:189 get_testfix_clause(testfix),
181 testfix = ''
182 return '%s%s%s %s' % (
183 testfix,
184 get_reviewer_clause(reviews),190 get_reviewer_clause(reviews),
185 get_bugs_clause(bugs),191 get_bugs_clause(bugs),
186 commit_text)192 get_qa_clause(bugs, no_qa,
193 incremental),
194 ])
195
196 return '%s %s' % (tags, commit_text)
187197
188198
189class Submitter(object):199class Submitter(object):
190200
191 def __init__(self, branch, location):201 def __init__(self, branch, location, testfix=False, no_qa=False,
202 incremental=False):
192 self.branch = branch203 self.branch = branch
204 self.testfix = testfix
205 self.no_qa = no_qa
206 self.incremental = incremental
193 self.config = self.branch.get_config()207 self.config = self.branch.get_config()
194 self.mail_to = self.config.get_user_option('pqm_email')208 self.mail_to = self.config.get_user_option('pqm_email')
195 if not self.mail_to:209 if not self.mail_to:
@@ -211,10 +225,11 @@
211 submission.check_public_branch()225 submission.check_public_branch()
212226
213 @staticmethod227 @staticmethod
214 def set_message(submission, mp):228 def set_message(submission, mp, testfix, no_qa, incremental):
215 pqm_command = ''.join(submission.to_lines())229 pqm_command = ''.join(submission.to_lines())
216 commit_message = mp.commit_message or ''230 commit_message = mp.commit_message or ''
217 start_message = mp.get_commit_message(commit_message)231 start_message = mp.get_commit_message(commit_message, testfix, no_qa,
232 incremental)
218 message = msgeditor.edit_commit_message(233 message = msgeditor.edit_commit_message(
219 'pqm command:\n%s' % pqm_command,234 'pqm command:\n%s' % pqm_command,
220 start_message=start_message).rstrip('\n')235 start_message=start_message).rstrip('\n')
@@ -227,7 +242,8 @@
227 mp = self.lander.load_merge_proposal(self.location)242 mp = self.lander.load_merge_proposal(self.location)
228 submission = self.submission(mp)243 submission = self.submission(mp)
229 self.check_submission(submission)244 self.check_submission(submission)
230 self.set_message(submission, mp)245 self.set_message(submission, mp, self.testfix, self.no_qa,
246 self.incremental)
231 email = submission.to_email(self.mail_from, self.mail_to)247 email = submission.to_email(self.mail_from, self.mail_to)
232 if outf is not None:248 if outf is not None:
233 outf.write(email.as_string())249 outf.write(email.as_string())
@@ -256,6 +272,39 @@
256 return '[bug=%s]' % ','.join(str(bug.id) for bug in bugs)272 return '[bug=%s]' % ','.join(str(bug.id) for bug in bugs)
257273
258274
275def get_testfix_clause(testfix=False):
276 """Get the testfix clause."""
277 if testfix:
278 testfix_clause = '[testfix]'
279 else:
280 testfix_clause = ''
281 return testfix_clause
282
283
284def get_qa_clause(bugs, no_qa=False, incremental=False):
285 """Check the no-qa and incremental options, getting the qa clause.
286
287 The qa clause will always be or no-qa, or incremental or no tags, never
288 both at the same time.
289 """
290 qa_clause = ""
291
292 if not bugs and not no_qa and not incremental:
293 raise MissingBugsError
294
295 if incremental and not bugs:
296 raise MissingBugsIncrementalError
297
298 if incremental:
299 qa_clause = '[incr]'
300 elif no_qa:
301 qa_clause = '[no-qa]'
302 else:
303 qa_clause = ''
304
305 return qa_clause
306
307
259def get_reviewer_handle(reviewer):308def get_reviewer_handle(reviewer):
260 """Get the handle for 'reviewer'.309 """Get the handle for 'reviewer'.
261310
262311
=== added directory 'tests'
=== added file 'tests/__init__.py'
=== added file 'tests/fakemethod.py'
--- tests/fakemethod.py 1970-01-01 00:00:00 +0000
+++ tests/fakemethod.py 2010-08-03 22:01:20 +0000
@@ -0,0 +1,21 @@
1__metaclass__ = type
2
3class FakeMethod:
4 """Catch any function or method call, and record the fact."""
5
6 def __init__(self, result=None, failure=None):
7 """Set up a fake function or method.
8
9 :param result: Value to return.
10 :param failure: Exception to raise.
11 """
12 self.result = result
13 self.failure = failure
14
15 def __call__(self, *args, **kwargs):
16 """Catch an invocation to the method."""
17
18 if self.failure is None:
19 return self.result
20 else:
21 raise self.failure
022
=== renamed file 'test_lpland.py' => 'tests/test_lpland.py'
--- test_lpland.py 2010-03-31 14:41:33 +0000
+++ tests/test_lpland.py 2010-08-03 22:01:20 +0000
@@ -13,7 +13,11 @@
1313
14from bzrlib.plugins.pqm.lpland import (14from bzrlib.plugins.pqm.lpland import (
15 get_bazaar_host, get_bugs_clause, get_reviewer_clause,15 get_bazaar_host, get_bugs_clause, get_reviewer_clause,
16 get_reviewer_handle, MissingReviewError)16 get_reviewer_handle, get_testfix_clause, get_qa_clause,
17 MissingReviewError, MissingBugsError, MissingBugsIncrementalError,
18 MergeProposal)
19
20from fakemethod import FakeMethod
1721
1822
19class FakeBug:23class FakeBug:
@@ -48,6 +52,16 @@
48 self.network = network52 self.network = network
4953
5054
55class FakeLPMergeProposal:
56 """Fake launchpadlib MergeProposal object.
57
58 Only used for the purposes of testing.
59 """
60
61 def __init__(self, root=None):
62 self._root = root
63
64
51class TestBugsClaused(unittest.TestCase):65class TestBugsClaused(unittest.TestCase):
52 """Tests for `get_bugs_clause`."""66 """Tests for `get_bugs_clause`."""
5367
@@ -70,6 +84,64 @@
70 self.assertEqual('[bug=20,45]', bugs_clause)84 self.assertEqual('[bug=20,45]', bugs_clause)
7185
7286
87class TestGetTestfixClause(unittest.TestCase):
88 """Tests for `get_testfix_clause`"""
89
90 def test_no_testfix(self):
91 testfix = False
92 self.assertEqual('', get_testfix_clause(testfix))
93
94 def test_is_testfix(self):
95 testfix = True
96 self.assertEqual('[testfix]', get_testfix_clause(testfix))
97
98
99class TestGetQaClause(unittest.TestCase):
100 """Tests for `get_qa_clause`"""
101
102 def test_no_bugs_no_option_given(self):
103 bugs = None
104 no_qa = False
105 incr = False
106 self.assertRaises(MissingBugsError, get_qa_clause, bugs, no_qa,
107 incr)
108
109 def test_bugs_noqa_option_given(self):
110 bug1 = FakeBug(20)
111 no_qa = True
112 incr = False
113 self.assertEqual('[no-qa]',
114 get_qa_clause([bug1], no_qa, incr))
115
116 def test_no_bugs_noqa_option_given(self):
117 bugs = None
118 no_qa = True
119 incr = False
120 self.assertEqual('[no-qa]',
121 get_qa_clause(bugs, no_qa, incr))
122
123 def test_bugs_no_option_given(self):
124 bug1 = FakeBug(20)
125 no_qa = False
126 incr = False
127 self.assertEqual('',
128 get_qa_clause([bug1], no_qa, incr))
129
130 def test_bugs_incr_option_given(self):
131 bug1 = FakeBug(20)
132 no_qa = False
133 incr = True
134 self.assertEqual('[incr]',
135 get_qa_clause([bug1], no_qa, incr))
136
137 def test_no_bugs_incr_option_given(self):
138 bugs = None
139 no_qa = False
140 incr = True
141 self.assertRaises(MissingBugsIncrementalError,
142 get_qa_clause, bugs, no_qa, incr)
143
144
73class TestGetReviewerHandle(unittest.TestCase):145class TestGetReviewerHandle(unittest.TestCase):
74 """Tests for `get_reviewer_handle`."""146 """Tests for `get_reviewer_handle`."""
75147
@@ -96,6 +168,91 @@
96 self.assertEqual('foo', get_reviewer_handle(person))168 self.assertEqual('foo', get_reviewer_handle(person))
97169
98170
171class TestGetCommitMessage(unittest.TestCase):
172
173 def setUp(self):
174 self.mp = MergeProposal(FakeLPMergeProposal())
175 self.fake_bug = FakeBug(20)
176 self.fake_person = self.makePerson('foo')
177
178 def makePerson(self, name):
179 return FakePerson(name, [])
180
181 def test_commit_with_bugs(self):
182 incr = False
183 no_qa = False
184 testfix = False
185
186 self.mp.get_bugs = FakeMethod([self.fake_bug])
187 self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
188
189 self.assertEqual("[r=foo][ui=none][bug=20] Foobaring the sbrubble.",
190 self.mp.get_commit_message("Foobaring the sbrubble.",
191 testfix, no_qa, incr))
192
193 def test_commit_no_bugs_no_noqa(self):
194 incr = False
195 no_qa = False
196 testfix = False
197
198 self.mp.get_bugs = FakeMethod([])
199 self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
200
201 self.assertRaises(MissingBugsError, self.mp.get_commit_message,
202 testfix, no_qa, incr)
203
204 def test_commit_no_bugs_with_noqa(self):
205 incr = False
206 no_qa = True
207 testfix = False
208
209 self.mp.get_bugs = FakeMethod([])
210 self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
211
212 self.assertEqual("[r=foo][ui=none][no-qa] Foobaring the sbrubble.",
213 self.mp.get_commit_message("Foobaring the sbrubble.",
214 testfix, no_qa, incr))
215
216 def test_commit_bugs_with_noqa(self):
217 incr = False
218 no_qa = True
219 testfix = False
220
221 self.mp.get_bugs = FakeMethod([self.fake_bug])
222 self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
223
224 self.assertEqual(
225 "[r=foo][ui=none][bug=20][no-qa] Foobaring the sbrubble.",
226 self.mp.get_commit_message("Foobaring the sbrubble.",
227 testfix, no_qa, incr))
228
229 def test_commit_bugs_with_incr(self):
230 incr = True
231 no_qa = False
232 testfix = False
233
234 self.mp.get_bugs = FakeMethod([self.fake_bug])
235 self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
236
237 self.assertEqual(
238 "[r=foo][ui=none][bug=20][incr] Foobaring the sbrubble.",
239 self.mp.get_commit_message("Foobaring the sbrubble.",
240 testfix, no_qa, incr))
241
242 def test_commit_no_bugs_with_incr(self):
243 incr = True
244 no_qa = False
245 testfix = False
246
247 self.mp.get_bugs = FakeMethod([self.fake_bug])
248 self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
249
250 self.assertEqual(
251 "[r=foo][ui=none][bug=20][incr] Foobaring the sbrubble.",
252 self.mp.get_commit_message("Foobaring the sbrubble.",
253 testfix, no_qa, incr))
254
255
99class TestGetReviewerClause(unittest.TestCase):256class TestGetReviewerClause(unittest.TestCase):
100 """Tests for `get_reviewer_clause`."""257 """Tests for `get_reviewer_clause`."""
101258
102259
=== renamed file 'test_pqm_submit.py' => 'tests/test_pqm_submit.py'

Subscribers

People subscribed via source and target branches

to all changes:
to status/vote changes: