Launchpad itself

Merge lp:~matsubara/launchpad/update-ec2-merge-workflow into lp:launchpad

  1. update-ec2-merge-workflow
  2. Merge into devel
Proposed by Diogo Matsubara
Status: Merged
Approved by: Diogo Matsubara
Approved revision: no longer in the source branch.
Merged at revision: 11502
Proposed branch: lp:~matsubara/launchpad/update-ec2-merge-workflow
Merge into: lp:launchpad
Diff against target: 167 lines (+65/-13)
3 files modified
lib/devscripts/autoland.py (+14/-7)
lib/devscripts/ec2test/builtins.py (+10/-6)
lib/devscripts/tests/test_autoland.py (+41/-0)
To merge this branch: bzr merge lp:~matsubara/launchpad/update-ec2-merge-workflow
Reviewer Review Type Date Requested Status
Abel Deuring (community) code Approve
Ursula Junque Pending
Review via email: mp+34486@code.launchpad.net

Commit message

Implements additional options to the ec2 land command: removes the restriction to use the --no-qa and --incremental option together and implements the --rollback option.

Description of the change

This branch implements additional options to the ec2 land command. More specifically it implements use cases 3 and 6 of https://dev.launchpad.net/QAProcessContinuousRollouts. It removes the restriction to use the --no-qa and --incremental option together and implements the --rollback option.

To post a comment you must log in.
Revision history for this message
Abel Deuring (adeuring) wrote :

(15:27:58) adeuring: matsubara: in lines 48..55 of the diff: if both (--no-qa or --incr) and --rollback=... are defined, the former options are ignored. Should we issue are warning in this case?
(15:29:37) salgado-brb heißt jetzt salgado
(15:30:57) matsubara: adeuring, you mean like raising an exception if --no-qa=True and -- incr=True and --rollback=123?
(15:31:15) matsubara: adeuring, sounds like a good idea, yes
(15:31:47) adeuring: matsubara: I don't know if this is worth an exception, just printing a user warning might be enough. But I leave decision to you ;)
(15:32:51) matsubara: adeuring, ok, I'll go with the exception as this is the same pattern used by the other options.
(15:32:59) adeuring: matsubara: OK
(15:41:24) adeuring: matsubara: in line 109, you catch a TypeError, probably raised in line 55, or somewhere in the option parser. But TypeErrors are quite generic -- could you move the "try: ... except TypeError:" directly to the place(s) where it occurs and raise the BzrComandError there? TypeError are so generic, and catching them not very close to the code where they are expected can hide completely unrelated errors.
(15:46:24) matsubara: adeuring, just a sec, I'm on the stand up call
(15:51:48) matsubara: adeuring, moving BzrCommandError to autoland.py wouldn't make much sense. I just tried something else instead and I think I can remove the exception handling
(15:52:19) adeuring: matsubara: ok
(15:52:48) matsubara: adeuring, since 67..71 I'm defining the --rollback option as int, if I pass something else like --rollback=foo, the option parser will return me a nice error message
(15:53:19) matsubara: so, I'll remove the exception handling from there as it's unecessary in that case.
(15:54:22) adeuring: matsubara: right, I wandered too if it was even necessary. (BTW, What happens for a missing parameter value)
(15:55:37) matsubara: adeuring, this https://pastebin.canonical.com/36722/
(15:57:46) adeuring: matsubara: so, that's an optparse issue -- let's not worry about it. I was just curious if this might cause the type error in 'rolback=%d'' % rollbak
(15:58:51) matsubara: adeuring, yep, I was being extra careful catching that TypeError as I thought 'rollback=%d'' % rollback' would raise that, but then optparse will take care of that for us.
(15:59:07) adeuring: exaxtly
(16:02:01) adeuring: matsubara: so, with "except TypeError:" removed and the other change about the options --no-qa and --rollback, r=me

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/devscripts/autoland.py'
--- lib/devscripts/autoland.py 2010-07-29 19:52:58 +0000
+++ lib/devscripts/autoland.py 2010-09-03 15:20:59 +0000
@@ -161,7 +161,7 @@
161 return URI(scheme='bzr+ssh', host=host, path='/' + branch.unique_name)161 return URI(scheme='bzr+ssh', host=host, path='/' + branch.unique_name)
162162
163 def get_commit_message(self, commit_text, testfix=False, no_qa=False,163 def get_commit_message(self, commit_text, testfix=False, no_qa=False,
164 incremental=False):164 incremental=False, rollback=None):
165 """Get the Launchpad-style commit message for a merge proposal."""165 """Get the Launchpad-style commit message for a merge proposal."""
166 reviews = self.get_reviews()166 reviews = self.get_reviews()
167 bugs = self.get_bugs()167 bugs = self.get_bugs()
@@ -171,7 +171,7 @@
171 get_reviewer_clause(reviews),171 get_reviewer_clause(reviews),
172 get_bugs_clause(bugs),172 get_bugs_clause(bugs),
173 get_qa_clause(bugs, no_qa,173 get_qa_clause(bugs, no_qa,
174 incremental),174 incremental, rollback=rollback),
175 ])175 ])
176176
177 return '%s %s' % (tags, commit_text)177 return '%s %s' % (tags, commit_text)
@@ -186,24 +186,31 @@
186 return testfix_clause186 return testfix_clause
187187
188188
189def get_qa_clause(bugs, no_qa=False, incremental=False):189def get_qa_clause(bugs, no_qa=False, incremental=False, rollback=None):
190 """Check the no-qa and incremental options, getting the qa clause.190 """Check the no-qa and incremental options, getting the qa clause.
191191
192 The qa clause will always be or no-qa, or incremental or no tags, never both at192 The qa clause will always be or no-qa, or incremental, or no-qa and
193 the same time.193 incremental, or a revno for the rollback clause, or no tags.
194
195 See https://dev.launchpad.net/QAProcessContinuousRollouts for detailed
196 explanation of each clause.
194 """197 """
195 qa_clause = ""198 qa_clause = ""
196199
197 if not bugs and not no_qa and not incremental:200 if not bugs and not no_qa and not incremental and not rollback:
198 raise MissingBugsError201 raise MissingBugsError
199202
200 if incremental and not bugs:203 if incremental and not bugs:
201 raise MissingBugsIncrementalError204 raise MissingBugsIncrementalError
202205
203 if incremental:206 if no_qa and incremental:
207 qa_clause = '[no-qa][incr]'
208 elif incremental:
204 qa_clause = '[incr]'209 qa_clause = '[incr]'
205 elif no_qa:210 elif no_qa:
206 qa_clause = '[no-qa]'211 qa_clause = '[no-qa]'
212 elif rollback:
213 qa_clause = '[rollback=%d]' % rollback
207 else:214 else:
208 qa_clause = ''215 qa_clause = ''
209216
210217
=== modified file 'lib/devscripts/ec2test/builtins.py'
--- lib/devscripts/ec2test/builtins.py 2010-08-18 17:40:29 +0000
+++ lib/devscripts/ec2test/builtins.py 2010-09-03 15:20:59 +0000
@@ -329,6 +329,11 @@
329 'incremental',329 'incremental',
330 help="Incremental to other bug fix (tags commit with [incr])."),330 help="Incremental to other bug fix (tags commit with [incr])."),
331 Option(331 Option(
332 'rollback', type=int,
333 help=(
334 "Rollback given revision number. (tags commit with "
335 "[rollback=revno]).")),
336 Option(
332 'commit-text', short_name='s', type=str,337 'commit-text', short_name='s', type=str,
333 help=(338 help=(
334 'A description of the landing, not including reviewer '339 'A description of the landing, not including reviewer '
@@ -361,8 +366,8 @@
361 def run(self, merge_proposal=None, machine=None,366 def run(self, merge_proposal=None, machine=None,
362 instance_type=DEFAULT_INSTANCE_TYPE, postmortem=False,367 instance_type=DEFAULT_INSTANCE_TYPE, postmortem=False,
363 debug=False, commit_text=None, dry_run=False, testfix=False,368 debug=False, commit_text=None, dry_run=False, testfix=False,
364 no_qa=False, incremental=False, print_commit=False, force=False,369 no_qa=False, incremental=False, rollback=None, print_commit=False,
365 attached=False):370 force=False, attached=False):
366 try:371 try:
367 from devscripts.autoland import (372 from devscripts.autoland import (
368 LaunchpadBranchLander, MissingReviewError, MissingBugsError,373 LaunchpadBranchLander, MissingReviewError, MissingBugsError,
@@ -382,9 +387,6 @@
382 if print_commit and dry_run:387 if print_commit and dry_run:
383 raise BzrCommandError(388 raise BzrCommandError(
384 "Cannot specify --print-commit and --dry-run.")389 "Cannot specify --print-commit and --dry-run.")
385 if no_qa and incremental:
386 raise BzrCommandError(
387 "--no-qa and --incremental cannot be given at the same time.")
388 lander = LaunchpadBranchLander.load()390 lander = LaunchpadBranchLander.load()
389391
390 if merge_proposal is None:392 if merge_proposal is None:
@@ -406,9 +408,11 @@
406 raise BzrCommandError(408 raise BzrCommandError(
407 "Commit text not specified. Use --commit-text, or specify a "409 "Commit text not specified. Use --commit-text, or specify a "
408 "message on the merge proposal.")410 "message on the merge proposal.")
411 if rollback and (no_qa or incremental):
412 print "--rollback option used. Ignoring --no-qa and --incremental."
409 try:413 try:
410 commit_message = mp.get_commit_message(414 commit_message = mp.get_commit_message(
411 commit_text, testfix, no_qa, incremental)415 commit_text, testfix, no_qa, incremental, rollback=rollback)
412 except MissingReviewError:416 except MissingReviewError:
413 raise BzrCommandError(417 raise BzrCommandError(
414 "Cannot land branches that haven't got approved code "418 "Cannot land branches that haven't got approved code "
415419
=== modified file 'lib/devscripts/tests/test_autoland.py'
--- lib/devscripts/tests/test_autoland.py 2010-07-29 19:53:32 +0000
+++ lib/devscripts/tests/test_autoland.py 2010-09-03 15:20:59 +0000
@@ -215,6 +215,28 @@
215 self.mp.get_commit_message("Foobaring the sbrubble.",215 self.mp.get_commit_message("Foobaring the sbrubble.",
216 testfix, no_qa, incr))216 testfix, no_qa, incr))
217217
218 def test_commit_with_noqa_and_incr(self):
219 incr = True
220 no_qa = True
221 testfix = False
222
223 self.mp.get_bugs = FakeMethod([self.fake_bug])
224 self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
225
226 self.assertEqual(
227 "[r=foo][ui=none][bug=20][no-qa][incr] Foobaring the sbrubble.",
228 self.mp.get_commit_message("Foobaring the sbrubble.",
229 testfix, no_qa, incr))
230
231 def test_commit_with_rollback(self):
232 self.mp.get_bugs = FakeMethod([self.fake_bug])
233 self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
234
235 self.assertEqual(
236 "[r=foo][ui=none][bug=20][rollback=123] Foobaring the sbrubble.",
237 self.mp.get_commit_message("Foobaring the sbrubble.",
238 rollback=123))
239
218240
219class TestGetTestfixClause(unittest.TestCase):241class TestGetTestfixClause(unittest.TestCase):
220 """Tests for `get_testfix_clause`"""242 """Tests for `get_testfix_clause`"""
@@ -273,6 +295,25 @@
273 self.assertRaises(MissingBugsIncrementalError,295 self.assertRaises(MissingBugsIncrementalError,
274 get_qa_clause, bugs, no_qa, incr)296 get_qa_clause, bugs, no_qa, incr)
275297
298 def test_bugs_incr_and_noqa_option_given(self):
299 bug1 = FakeBug(20)
300 no_qa = True
301 incr = True
302 self.assertEqual('[no-qa][incr]',
303 get_qa_clause([bug1], no_qa, incr))
304
305 def test_rollback_given(self):
306 bugs = None
307 self.assertEqual('[rollback=123]',
308 get_qa_clause(bugs, rollback=123))
309
310 def test_rollback_and_noqa_and_incr_given(self):
311 bugs = None
312 no_qa = True
313 incr = True
314 self.assertEqual('[rollback=123]',
315 get_qa_clause(bugs, rollback=123))
316
276317
277class TestGetReviewerHandle(unittest.TestCase):318class TestGetReviewerHandle(unittest.TestCase):
278 """Tests for `get_reviewer_handle`."""319 """Tests for `get_reviewer_handle`."""