Merge lp:~statik/tarmac/reject-failures into lp:tarmac

Proposed by Elliot Murphy
Status: Merged
Merged at revision: not available
Proposed branch: lp:~statik/tarmac/reject-failures
Merge into: lp:tarmac
Diff against target: None lines
To merge this branch: bzr merge lp:~statik/tarmac/reject-failures
Reviewer Review Type Date Requested Status
Paul Hummer Needs Fixing
Review via email:
To post a comment you must log in.
Revision history for this message
Elliot Murphy (statik) wrote :

Hi Paul,

An important thing to do if the branch fails to merge is to take it out of the approved queue. I think this branch does that, although I'm uncertain about whether setting queue_status works as I expected it to.

Revision history for this message
Paul Hummer (rockstar) wrote :

Hi statik-

  Strangely enough, I was planning on doing this EXACT same thing tonight.
Thanks for taking care of it for me. There's only one issue with this patch,
and I've documented it below. I'm willing to chat about this though, so don't
think it's not open for discussion.

=== modified file 'tarmac/'
--- a/tarmac/ 2009-03-13 01:56:16 +0000
+++ b/tarmac/ 2009-03-13 21:52:50 +0000
@@ -140,6 +145,12 @@
                     if self.dry_run:
                         print 'Branch failed test command'
+ comment = '\n'.join(['Branch failed test command',
+ stdout_value, stderr_value])
+ candidate.createComment(subject="unused", content=comment,
+ vote='Needs Fixing')
+ candidate.queue_status = 'Needs Fixing'
+ candidate.lp_save()

Just a couple of things. I think the subject should be something like 'Branch
failed test command', and then the comment should just be stdout and stderr.
I'm also not sure if we should add the vote here, or just change the status.
All strings need to be unicode (although it still shouldn't affect it). The
queue_status is actually u'Needs review'. I've been trying to brainstorm a
better way to do enumerators, but for now, we just kinda gotta memorize them.

review: Needs Fixing
Revision history for this message
Elliot Murphy (statik) wrote :

Fixed, and pushed revision 55. Thanks for the great feedback!

lp:~statik/tarmac/reject-failures updated
55. By Elliot Murphy

Implemented review feedback.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'tarmac/'
2--- tarmac/ 2009-03-13 01:56:16 +0000
3+++ tarmac/ 2009-03-13 21:33:40 +0000
4@@ -129,7 +129,12 @@
5 if self.test_command:
6 cwd = os.getcwd()
7 os.chdir(temp_dir)
8- retcode =, shell=True)
9+ proc = subprocess.Popen(self.test_command,
10+ shell=True,
11+ stdout=subprocess.PIPE,
12+ stderr=subprocess.PIPE)
13+ stdout_value, stderr_value = proc.communicate()
14+ retcode = proc.wait()
15 os.chdir(cwd)
16 if retcode == 0:
17 if not self.dry_run:
18@@ -140,6 +145,12 @@
19 if self.dry_run:
20 print 'Branch failed test command'
21 target_tree.revert()
22+ comment = '\n'.join(['Branch failed test command',
23+ stdout_value, stderr_value])
24+ candidate.createComment(subject="unused", content=comment,
25+ vote='Needs Fixing')
26+ candidate.queue_status = 'Needs Fixing'
27+ candidate.lp_save()
28 else:
29 if not self.dry_run:
30 target_tree.commit(commit_message)


People subscribed via source and target branches