Merge lp:~james-w/tarmac/break-up-do-merges into lp:~ubuntuone-hackers/tarmac/trunk
- break-up-do-merges
- Merge into trunk
Proposed by
James Westby
Status: | Merged |
---|---|
Merged at revision: | 420 |
Proposed branch: | lp:~james-w/tarmac/break-up-do-merges |
Merge into: | lp:~ubuntuone-hackers/tarmac/trunk |
Prerequisite: | lp:~james-w/tarmac/commit-message |
Diff against target: |
453 lines (+206/-191) 1 file modified
tarmac/bin/commands.py (+206/-191) |
To merge this branch: | bzr merge lp:~james-w/tarmac/break-up-do-merges |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Jonathan Lange (community) | Needs Fixing | ||
Review via email: mp+140554@code.launchpad.net |
This proposal supersedes a proposal from 2012-12-18.
Commit message
Description of the change
Hi,
This branch breaks up the _do_merges method a little, as it was huge.
I tried to keep changes to a minimum while moving the code around, but it
used "continue" for flow control, so I had to change it a little to
use return values.
Thanks,
James
To post a comment you must log in.
Revision history for this message
Jonathan Lange (jml) wrote : | # |
Revision history for this message
Jonathan Lange (jml) : | # |
review:
Needs Fixing
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'tarmac/bin/commands.py' |
2 | --- tarmac/bin/commands.py 2012-09-05 18:08:50 +0000 |
3 | +++ tarmac/bin/commands.py 2012-12-18 21:25:42 +0000 |
4 | @@ -134,6 +134,194 @@ |
5 | help_commands(self.outf) |
6 | |
7 | |
8 | +def _get_mergable_proposals_for_branch(lp_branch, logger, imply_commit_message=False): |
9 | + """Return a list of the mergable proposals for the given branch.""" |
10 | + proposals = [] |
11 | + for entry in lp_branch.landing_candidates: |
12 | + logger.debug("Considering merge proposal: {0}".format(entry.web_link)) |
13 | + |
14 | + if entry.queue_status != u'Approved': |
15 | + logger.debug( |
16 | + " Skipping proposal: status is {0}, not " |
17 | + "'Approved'".format(entry.queue_status)) |
18 | + continue |
19 | + |
20 | + if (not imply_commit_message and not entry.commit_message): |
21 | + logger.debug( |
22 | + " Skipping proposal: proposal has no commit message") |
23 | + continue |
24 | + |
25 | + proposals.append(entry) |
26 | + return proposals |
27 | + |
28 | + |
29 | +def _get_reviews(proposal): |
30 | + """Get the set of reviews from the proposal.""" |
31 | + reviews = [] |
32 | + for vote in proposal.votes: |
33 | + if not vote.comment: |
34 | + continue |
35 | + else: |
36 | + reviews.append('%s;%s' % (vote.reviewer.display_name, |
37 | + vote.comment.vote)) |
38 | + |
39 | + if len(reviews) == 0: |
40 | + return None |
41 | + |
42 | + return reviews |
43 | + |
44 | + |
45 | +def setup(logger, debug=False, http_debug=False): |
46 | + if debug: |
47 | + set_up_debug_logging() |
48 | + logger.debug('Debug logging enabled') |
49 | + if http_debug: |
50 | + httplib2.debuglevel = 1 |
51 | + logger.debug('HTTP debugging enabled.') |
52 | + logger.debug('Loading plugins') |
53 | + load_plugins() |
54 | + logger.debug('Plugins loaded') |
55 | + |
56 | + |
57 | +def merge_proposals(target, proposals, logger, config, command): |
58 | + logger.debug('Firing tarmac_pre_merge hook') |
59 | + tarmac_hooks.fire('tarmac_pre_merge', |
60 | + command, target) |
61 | + |
62 | + statuses = [] |
63 | + try: |
64 | + for proposal in proposals: |
65 | + status = merge_proposal(target, proposal, logger, |
66 | + config, command) |
67 | + statuses.append(status) |
68 | + # This except is here because we need the else and can't have it |
69 | + # without an except as well. |
70 | + except: |
71 | + raise |
72 | + else: |
73 | + logger.debug('Firing tarmac_post_merge hook') |
74 | + tarmac_hooks.fire('tarmac_post_merge', |
75 | + command, target) |
76 | + finally: |
77 | + target.cleanup() |
78 | + return statuses |
79 | + |
80 | + |
81 | +def merge_proposal(target, proposal, logger, config, command): |
82 | + target.cleanup() |
83 | + logger.debug( |
84 | + u'Preparing to merge %(source_branch)s' % { |
85 | + 'source_branch': proposal.source_branch.web_link}) |
86 | + try: |
87 | + prerequisite = proposal.prerequisite_branch |
88 | + if prerequisite: |
89 | + merges = [x for x in prerequisite.landing_targets |
90 | + if x.target_branch == target.lp_branch and |
91 | + x.queue_status != u'Superseded'] |
92 | + if len(merges) == 0: |
93 | + raise TarmacMergeError( |
94 | + u'No proposals of prerequisite branch.', |
95 | + u'No proposals found for merge of %s ' |
96 | + u'into %s.' % ( |
97 | + prerequisite.web_link, |
98 | + target.lp_branch.web_link)) |
99 | + elif len(merges) > 1: |
100 | + raise TarmacMergeError( |
101 | + u'Too many proposals of prerequisite.', |
102 | + u'More than one proposal found for merge ' |
103 | + u'of %s into %s, which is not Superseded.' % ( |
104 | + prerequisite.web_link, |
105 | + target.lp_branch.web_link)) |
106 | + elif len(merges) == 1: |
107 | + if merges[0].queue_status != u'Merged': |
108 | + raise TarmacMergeError( |
109 | + u'Prerequisite not yet merged.', |
110 | + u'The prerequisite %s has not yet been ' |
111 | + u'merged into %s.' % ( |
112 | + prerequisite.web_link, |
113 | + target.lp_branch.web_link)) |
114 | + |
115 | + if not proposal.reviewed_revid: |
116 | + raise TarmacMergeError( |
117 | + u'No approved revision specified.') |
118 | + |
119 | + |
120 | + source = Branch.create( |
121 | + proposal.source_branch, config, target=target) |
122 | + |
123 | + source_bzr_branch = source.get_bzr_branch() |
124 | + approved = source_bzr_branch.revision_id_to_revno( |
125 | + str(proposal.reviewed_revid)) |
126 | + tip = source_bzr_branch.revno() |
127 | + |
128 | + if tip > approved: |
129 | + message = u'Unapproved changes made after approval' |
130 | + lp_comment = ( |
131 | + u'There are additional revisions which have not ' |
132 | + u'been approved in review. Please seek review and ' |
133 | + u'approval of these new revisions.') |
134 | + raise UnapprovedChanges(message, lp_comment) |
135 | + |
136 | + logger.debug( |
137 | + 'Merging %(source)s at revision %(revision)s' % { |
138 | + 'source': proposal.source_branch.web_link, |
139 | + 'revision': proposal.reviewed_revid}) |
140 | + |
141 | + target.merge(source, str(proposal.reviewed_revid)) |
142 | + |
143 | + logger.debug('Firing tarmac_pre_commit hook') |
144 | + tarmac_hooks.fire('tarmac_pre_commit', |
145 | + command, target, source, proposal) |
146 | + |
147 | + except TarmacMergeError, failure: |
148 | + logger.warn( |
149 | + u'Merging %(source)s into %(target)s failed: %(msg)s' % |
150 | + {'source': proposal.source_branch.web_link, |
151 | + 'target': proposal.target_branch.web_link, |
152 | + 'msg': str(failure)}) |
153 | + |
154 | + subject = u'Re: [Merge] %(source)s into %(target)s' % { |
155 | + "source": proposal.source_branch.display_name, |
156 | + "target": proposal.target_branch.display_name} |
157 | + |
158 | + if failure.comment: |
159 | + comment = failure.comment |
160 | + else: |
161 | + comment = str(failure) |
162 | + |
163 | + proposal.createComment(subject=subject, content=comment) |
164 | + try: |
165 | + proposal.setStatus( |
166 | + status=config.rejected_branch_status) |
167 | + except AttributeError: |
168 | + proposal.setStatus(status=u'Needs review') |
169 | + proposal.lp_save() |
170 | + return False |
171 | + |
172 | + except PointlessMerge: |
173 | + logger.warn( |
174 | + 'Merging %(source)s into %(target)s would be ' |
175 | + 'pointless.' % { |
176 | + 'source': proposal.source_branch.web_link, |
177 | + 'target': proposal.target_branch.web_link}) |
178 | + return False |
179 | + |
180 | + merge_url = get_review_url(proposal) |
181 | + revprops = {'merge_url': merge_url} |
182 | + |
183 | + commit_message = proposal.commit_message |
184 | + if commit_message is None and config.imply_commit_message: |
185 | + commit_message = proposal.description |
186 | + |
187 | + target.commit(commit_message, |
188 | + reviews=_get_reviews(proposal)) |
189 | + |
190 | + logger.debug('Firing tarmac_post_commit hook') |
191 | + tarmac_hooks.fire('tarmac_post_commit', |
192 | + command, target, source, proposal) |
193 | + return True |
194 | + |
195 | + |
196 | class cmd_merge(TarmacCommand): |
197 | '''Automatically merge approved merge proposal branches.''' |
198 | |
199 | @@ -143,7 +331,8 @@ |
200 | options.http_debug_option, |
201 | options.debug_option, |
202 | options.imply_commit_message_option, |
203 | - options.one_option] |
204 | + options.one_option, |
205 | + ] |
206 | |
207 | def _do_merges(self, branch_url): |
208 | |
209 | @@ -152,204 +341,29 @@ |
210 | self.logger.info('Not a valid branch: {0}'.format(branch_url)) |
211 | return |
212 | |
213 | - proposals = self._get_mergable_proposals_for_branch(lp_branch) |
214 | + proposals = _get_mergable_proposals_for_branch(lp_branch, self.logger, |
215 | + imply_commit_message=self.config.imply_commit_message) |
216 | |
217 | if not proposals: |
218 | self.logger.info( |
219 | 'No approved proposals found for %(branch_url)s' % { |
220 | 'branch_url': branch_url}) |
221 | - return |
222 | + return [] |
223 | + |
224 | + if self.config.one: |
225 | + proposals = proposals[0] |
226 | |
227 | target = Branch.create(lp_branch, self.config, create_tree=True) |
228 | - |
229 | - self.logger.debug('Firing tarmac_pre_merge hook') |
230 | - tarmac_hooks.fire('tarmac_pre_merge', |
231 | - self, target) |
232 | - |
233 | - try: |
234 | - for proposal in proposals: |
235 | - target.cleanup() |
236 | - self.logger.debug( |
237 | - u'Preparing to merge %(source_branch)s' % { |
238 | - 'source_branch': proposal.source_branch.web_link}) |
239 | - try: |
240 | - prerequisite = proposal.prerequisite_branch |
241 | - if prerequisite: |
242 | - merges = [x for x in prerequisite.landing_targets |
243 | - if x.target_branch == target.lp_branch and |
244 | - x.queue_status != u'Superseded'] |
245 | - if len(merges) == 0: |
246 | - raise TarmacMergeError( |
247 | - u'No proposals of prerequisite branch.', |
248 | - u'No proposals found for merge of %s ' |
249 | - u'into %s.' % ( |
250 | - prerequisite.web_link, |
251 | - target.lp_branch.web_link)) |
252 | - elif len(merges) > 1: |
253 | - raise TarmacMergeError( |
254 | - u'Too many proposals of prerequisite.', |
255 | - u'More than one proposal found for merge ' |
256 | - u'of %s into %s, which is not Superseded.' % ( |
257 | - prerequisite.web_link, |
258 | - target.lp_branch.web_link)) |
259 | - elif len(merges) == 1: |
260 | - if merges[0].queue_status != u'Merged': |
261 | - raise TarmacMergeError( |
262 | - u'Prerequisite not yet merged.', |
263 | - u'The prerequisite %s has not yet been ' |
264 | - u'merged into %s.' % ( |
265 | - prerequisite.web_link, |
266 | - target.lp_branch.web_link)) |
267 | - |
268 | - if not proposal.reviewed_revid: |
269 | - raise TarmacMergeError( |
270 | - u'No approved revision specified.') |
271 | - |
272 | - |
273 | - source = Branch.create( |
274 | - proposal.source_branch, self.config, target=target) |
275 | - |
276 | - source_bzr_branch = source.get_bzr_branch() |
277 | - approved = source_bzr_branch.revision_id_to_revno( |
278 | - str(proposal.reviewed_revid)) |
279 | - tip = source_bzr_branch.revno() |
280 | - |
281 | - if tip > approved: |
282 | - message = u'Unapproved changes made after approval' |
283 | - lp_comment = ( |
284 | - u'There are additional revisions which have not ' |
285 | - u'been approved in review. Please seek review and ' |
286 | - u'approval of these new revisions.') |
287 | - raise UnapprovedChanges(message, lp_comment) |
288 | - |
289 | - self.logger.debug( |
290 | - 'Merging %(source)s at revision %(revision)s' % { |
291 | - 'source': proposal.source_branch.web_link, |
292 | - 'revision': proposal.reviewed_revid}) |
293 | - |
294 | - target.merge(source, str(proposal.reviewed_revid)) |
295 | - |
296 | - self.logger.debug('Firing tarmac_pre_commit hook') |
297 | - tarmac_hooks.fire('tarmac_pre_commit', |
298 | - self, target, source, proposal) |
299 | - |
300 | - except TarmacMergeError, failure: |
301 | - self.logger.warn( |
302 | - u'Merging %(source)s into %(target)s failed: %(msg)s' % |
303 | - {'source': proposal.source_branch.web_link, |
304 | - 'target': proposal.target_branch.web_link, |
305 | - 'msg': str(failure)}) |
306 | - |
307 | - subject = u'Re: [Merge] %(source)s into %(target)s' % { |
308 | - "source": proposal.source_branch.display_name, |
309 | - "target": proposal.target_branch.display_name} |
310 | - |
311 | - if failure.comment: |
312 | - comment = failure.comment |
313 | - else: |
314 | - comment = str(failure) |
315 | - |
316 | - proposal.createComment(subject=subject, content=comment) |
317 | - try: |
318 | - proposal.setStatus( |
319 | - status=self.config.rejected_branch_status) |
320 | - except AttributeError: |
321 | - proposal.setStatus(status=u'Needs review') |
322 | - proposal.lp_save() |
323 | - |
324 | - # If we've been asked to only merge one branch, then exit. |
325 | - if self.config.one: |
326 | - return True |
327 | - |
328 | - continue |
329 | - except PointlessMerge: |
330 | - self.logger.warn( |
331 | - 'Merging %(source)s into %(target)s would be ' |
332 | - 'pointless.' % { |
333 | - 'source': proposal.source_branch.web_link, |
334 | - 'target': proposal.target_branch.web_link}) |
335 | - continue |
336 | - |
337 | - merge_url = get_review_url(proposal) |
338 | - revprops = {'merge_url': merge_url} |
339 | - |
340 | - commit_message = proposal.commit_message |
341 | - if commit_message is None and self.config.imply_commit_message: |
342 | - commit_message = proposal.description |
343 | - |
344 | - target.commit(commit_message, |
345 | - revprops=revprops, |
346 | - authors=source.authors, |
347 | - reviews=self._get_reviews(proposal)) |
348 | - |
349 | - self.logger.debug('Firing tarmac_post_commit hook') |
350 | - tarmac_hooks.fire('tarmac_post_commit', |
351 | - self, target, source, proposal) |
352 | - |
353 | - # If we've been asked to only merge one branch, then exit. |
354 | - if self.config.one: |
355 | - return True |
356 | - |
357 | - # This except is here because we need the else and can't have it |
358 | - # without an except as well. |
359 | - except: |
360 | - raise |
361 | - else: |
362 | - self.logger.debug('Firing tarmac_post_merge hook') |
363 | - tarmac_hooks.fire('tarmac_post_merge', |
364 | - self, target) |
365 | - finally: |
366 | - target.cleanup() |
367 | - |
368 | - def _get_mergable_proposals_for_branch(self, lp_branch): |
369 | - """Return a list of the mergable proposals for the given branch.""" |
370 | - proposals = [] |
371 | - for entry in lp_branch.landing_candidates: |
372 | - self.logger.debug("Considering merge proposal: {0}".format(entry.web_link)) |
373 | - |
374 | - if entry.queue_status != u'Approved': |
375 | - self.logger.debug( |
376 | - " Skipping proposal: status is {0}, not " |
377 | - "'Approved'".format(entry.queue_status)) |
378 | - continue |
379 | - |
380 | - if (not self.config.imply_commit_message and |
381 | - not entry.commit_message): |
382 | - self.logger.debug( |
383 | - " Skipping proposal: proposal has no commit message") |
384 | - continue |
385 | - |
386 | - proposals.append(entry) |
387 | - return proposals |
388 | - |
389 | - def _get_reviews(self, proposal): |
390 | - """Get the set of reviews from the proposal.""" |
391 | - reviews = [] |
392 | - for vote in proposal.votes: |
393 | - if not vote.comment: |
394 | - continue |
395 | - else: |
396 | - reviews.append('%s;%s' % (vote.reviewer.display_name, |
397 | - vote.comment.vote)) |
398 | - |
399 | - if len(reviews) == 0: |
400 | - return None |
401 | - |
402 | - return reviews |
403 | + statuses = merge_proposals(target, proposals, self.logger, self.config, self) |
404 | + return statuses |
405 | + |
406 | |
407 | def run(self, branch_url=None, launchpad=None, **kwargs): |
408 | for key, value in kwargs.iteritems(): |
409 | self.config.set('Tarmac', key, value) |
410 | |
411 | - if self.config.debug: |
412 | - set_up_debug_logging() |
413 | - self.logger.debug('Debug logging enabled') |
414 | - if self.config.http_debug: |
415 | - httplib2.debuglevel = 1 |
416 | - self.logger.debug('HTTP debugging enabled.') |
417 | - self.logger.debug('Loading plugins') |
418 | - load_plugins() |
419 | - self.logger.debug('Plugins loaded') |
420 | + setup(self.logger, debug=self.config.debug, |
421 | + http_debug=self.config.http_debug) |
422 | |
423 | self.launchpad = launchpad |
424 | if self.launchpad is None: |
425 | @@ -357,23 +371,24 @@ |
426 | self.launchpad = self.get_launchpad_object() |
427 | self.logger.debug('launchpad object loaded') |
428 | |
429 | + statuses = [] |
430 | + |
431 | if branch_url: |
432 | self.logger.debug('%(branch_url)s specified as branch_url' % { |
433 | 'branch_url': branch_url}) |
434 | if not branch_url.startswith('lp:'): |
435 | raise TarmacCommandError('Branch urls must start with lp:') |
436 | - self._do_merges(branch_url) |
437 | - |
438 | + statuses.extend(self._do_merges(branch_url)) |
439 | else: |
440 | for branch in self.config.branches: |
441 | self.logger.debug( |
442 | 'Merging approved branches against %(branch)s' % { |
443 | 'branch': branch}) |
444 | try: |
445 | - merged = self._do_merges(branch) |
446 | + statuses.extend(self._do_merges(branch)) |
447 | |
448 | # If we've been asked to only merge one branch, then exit. |
449 | - if merged and self.config.one: |
450 | + if statuses and self.config.one: |
451 | break |
452 | except LockContention: |
453 | continue |
On 18 December 2012 21:26, James Westby <email address hidden> wrote: /code.launchpad .net/~james- w/tarmac/ break-up- do-merges/ +merge/ 140554
> James Westby has proposed merging lp:~james-w/tarmac/break-up-do-merges into lp:~ubuntuone-hackers/tarmac/trunk with lp:~james-w/tarmac/commit-message as a prerequisite.
>
> Requested reviews:
> Ubuntu One hackers (ubuntuone-hackers)
>
> For more details, see:
> https:/
>
> Hi,
>
> This branch breaks up the _do_merges method a little, as it was huge.
>
It was!
See inline.
> I tried to keep changes to a minimum while moving the code around, but it
> used "continue" for flow control, so I had to change it a little to
> use return values.
>
... bin/commands. py' bin/commands. py 2012-09-05 18:08:50 +0000 bin/commands. py 2012-12-18 21:25:42 +0000 self.outf) proposals_ for_branch( lp_branch, logger, imply_commit_ message= False): landing_ candidates: debug(" Considering merge proposal: {0}".format( entry.web_ link)) ".format( entry.queue_ status) ) message and not entry.commit_ message) : append( entry)
> === modified file 'tarmac/
> --- tarmac/
> +++ tarmac/
> @@ -134,6 +134,194 @@
> help_commands(
>
>
> +def _get_mergable_
> + """Return a list of the mergable proposals for the given branch."""
> + proposals = []
> + for entry in lp_branch.
> + logger.
> +
> + if entry.queue_status != u'Approved':
> + logger.debug(
> + " Skipping proposal: status is {0}, not "
> + "'Approved'
> + continue
> +
> + if (not imply_commit_
> + logger.debug(
> + " Skipping proposal: proposal has no commit message")
> + continue
> +
> + proposals.
> + return proposals
> +
This seems a natural candidate for an iterator. Still, no big deal.
> +def _get_reviews( proposal) : append( '%s;%s' % (vote.reviewer. display_ name,
> + """Get the set of reviews from the proposal."""
> + reviews = []
> + for vote in proposal.votes:
> + if not vote.comment:
> + continue
> + else:
> + reviews.
> + vote.comment.vote))
> +
> + if len(reviews) == 0:
> + return None
> +
> + return reviews
> +
Again, a potential iterator, and again, I don't care enough for this branch.
Implementation could probably be more functional, but even this is a
huge step forwards.
> +
> +def setup(logger, debug=False, http_debug=False):
"setup" is a noun, "set up" a verb phrase. This should be spelled
"set_up". Also, it should probably say what it's going to set up.
> +def merge_proposals (target, proposals, logger, config, command): debug(' Firing tarmac_pre_merge hook') hooks.fire( 'tarmac_ pre_merge' , target, proposal, logger, append( status)
> + logger.
> + tarmac_
> + command, target)
> +
> + statuses = []
> + try:
> + for proposal in proposals:
> + status = merge_proposal(
> + config, command)
> + statuses.
> + # This except is here because we need the else and can't have it
> + # without an except as well.
Wat.
try:
thing()
except:
raise
else:
another_thing()
...