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
1=== modified file 'lib/devscripts/autoland.py'
2--- lib/devscripts/autoland.py 2010-07-29 19:52:58 +0000
3+++ lib/devscripts/autoland.py 2010-09-03 15:20:59 +0000
4@@ -161,7 +161,7 @@
5 return URI(scheme='bzr+ssh', host=host, path='/' + branch.unique_name)
6
7 def get_commit_message(self, commit_text, testfix=False, no_qa=False,
8- incremental=False):
9+ incremental=False, rollback=None):
10 """Get the Launchpad-style commit message for a merge proposal."""
11 reviews = self.get_reviews()
12 bugs = self.get_bugs()
13@@ -171,7 +171,7 @@
14 get_reviewer_clause(reviews),
15 get_bugs_clause(bugs),
16 get_qa_clause(bugs, no_qa,
17- incremental),
18+ incremental, rollback=rollback),
19 ])
20
21 return '%s %s' % (tags, commit_text)
22@@ -186,24 +186,31 @@
23 return testfix_clause
24
25
26-def get_qa_clause(bugs, no_qa=False, incremental=False):
27+def get_qa_clause(bugs, no_qa=False, incremental=False, rollback=None):
28 """Check the no-qa and incremental options, getting the qa clause.
29
30- The qa clause will always be or no-qa, or incremental or no tags, never both at
31- the same time.
32+ The qa clause will always be or no-qa, or incremental, or no-qa and
33+ incremental, or a revno for the rollback clause, or no tags.
34+
35+ See https://dev.launchpad.net/QAProcessContinuousRollouts for detailed
36+ explanation of each clause.
37 """
38 qa_clause = ""
39
40- if not bugs and not no_qa and not incremental:
41+ if not bugs and not no_qa and not incremental and not rollback:
42 raise MissingBugsError
43
44 if incremental and not bugs:
45 raise MissingBugsIncrementalError
46
47- if incremental:
48+ if no_qa and incremental:
49+ qa_clause = '[no-qa][incr]'
50+ elif incremental:
51 qa_clause = '[incr]'
52 elif no_qa:
53 qa_clause = '[no-qa]'
54+ elif rollback:
55+ qa_clause = '[rollback=%d]' % rollback
56 else:
57 qa_clause = ''
58
59
60=== modified file 'lib/devscripts/ec2test/builtins.py'
61--- lib/devscripts/ec2test/builtins.py 2010-08-18 17:40:29 +0000
62+++ lib/devscripts/ec2test/builtins.py 2010-09-03 15:20:59 +0000
63@@ -329,6 +329,11 @@
64 'incremental',
65 help="Incremental to other bug fix (tags commit with [incr])."),
66 Option(
67+ 'rollback', type=int,
68+ help=(
69+ "Rollback given revision number. (tags commit with "
70+ "[rollback=revno]).")),
71+ Option(
72 'commit-text', short_name='s', type=str,
73 help=(
74 'A description of the landing, not including reviewer '
75@@ -361,8 +366,8 @@
76 def run(self, merge_proposal=None, machine=None,
77 instance_type=DEFAULT_INSTANCE_TYPE, postmortem=False,
78 debug=False, commit_text=None, dry_run=False, testfix=False,
79- no_qa=False, incremental=False, print_commit=False, force=False,
80- attached=False):
81+ no_qa=False, incremental=False, rollback=None, print_commit=False,
82+ force=False, attached=False):
83 try:
84 from devscripts.autoland import (
85 LaunchpadBranchLander, MissingReviewError, MissingBugsError,
86@@ -382,9 +387,6 @@
87 if print_commit and dry_run:
88 raise BzrCommandError(
89 "Cannot specify --print-commit and --dry-run.")
90- if no_qa and incremental:
91- raise BzrCommandError(
92- "--no-qa and --incremental cannot be given at the same time.")
93 lander = LaunchpadBranchLander.load()
94
95 if merge_proposal is None:
96@@ -406,9 +408,11 @@
97 raise BzrCommandError(
98 "Commit text not specified. Use --commit-text, or specify a "
99 "message on the merge proposal.")
100+ if rollback and (no_qa or incremental):
101+ print "--rollback option used. Ignoring --no-qa and --incremental."
102 try:
103 commit_message = mp.get_commit_message(
104- commit_text, testfix, no_qa, incremental)
105+ commit_text, testfix, no_qa, incremental, rollback=rollback)
106 except MissingReviewError:
107 raise BzrCommandError(
108 "Cannot land branches that haven't got approved code "
109
110=== modified file 'lib/devscripts/tests/test_autoland.py'
111--- lib/devscripts/tests/test_autoland.py 2010-07-29 19:53:32 +0000
112+++ lib/devscripts/tests/test_autoland.py 2010-09-03 15:20:59 +0000
113@@ -215,6 +215,28 @@
114 self.mp.get_commit_message("Foobaring the sbrubble.",
115 testfix, no_qa, incr))
116
117+ def test_commit_with_noqa_and_incr(self):
118+ incr = True
119+ no_qa = True
120+ testfix = False
121+
122+ self.mp.get_bugs = FakeMethod([self.fake_bug])
123+ self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
124+
125+ self.assertEqual(
126+ "[r=foo][ui=none][bug=20][no-qa][incr] Foobaring the sbrubble.",
127+ self.mp.get_commit_message("Foobaring the sbrubble.",
128+ testfix, no_qa, incr))
129+
130+ def test_commit_with_rollback(self):
131+ self.mp.get_bugs = FakeMethod([self.fake_bug])
132+ self.mp.get_reviews = FakeMethod({None : [self.fake_person]})
133+
134+ self.assertEqual(
135+ "[r=foo][ui=none][bug=20][rollback=123] Foobaring the sbrubble.",
136+ self.mp.get_commit_message("Foobaring the sbrubble.",
137+ rollback=123))
138+
139
140 class TestGetTestfixClause(unittest.TestCase):
141 """Tests for `get_testfix_clause`"""
142@@ -273,6 +295,25 @@
143 self.assertRaises(MissingBugsIncrementalError,
144 get_qa_clause, bugs, no_qa, incr)
145
146+ def test_bugs_incr_and_noqa_option_given(self):
147+ bug1 = FakeBug(20)
148+ no_qa = True
149+ incr = True
150+ self.assertEqual('[no-qa][incr]',
151+ get_qa_clause([bug1], no_qa, incr))
152+
153+ def test_rollback_given(self):
154+ bugs = None
155+ self.assertEqual('[rollback=123]',
156+ get_qa_clause(bugs, rollback=123))
157+
158+ def test_rollback_and_noqa_and_incr_given(self):
159+ bugs = None
160+ no_qa = True
161+ incr = True
162+ self.assertEqual('[rollback=123]',
163+ get_qa_clause(bugs, rollback=123))
164+
165
166 class TestGetReviewerHandle(unittest.TestCase):
167 """Tests for `get_reviewer_handle`."""