Tarmac

Merge lp:~jml/tarmac/handle-private-branch into lp:~ubuntuone-hackers/tarmac/trunk

  1. handle-private-branch
  2. Merge into trunk
Proposed by Jonathan Lange
Status: Merged
Approved by: Sidnei da Silva
Approved revision: 433
Merge reported by: Sidnei da Silva
Merged at revision: not available
Proposed branch: lp:~jml/tarmac/handle-private-branch
Merge into: lp:~ubuntuone-hackers/tarmac/trunk
Diff against target: 173 lines (+75/-27)
2 files modified
tarmac/bin/commands.py (+58/-26)
tarmac/tests/test_commands.py (+17/-1)
To merge this branch: bzr merge lp:~jml/tarmac/handle-private-branch
Reviewer Review Type Date Requested Status
Natalia Bidart (community) Approve
Review via email: mp+141910@code.launchpad.net

Description of the change

We're getting this error from tarmac on our jenkins server.

10:21:03 E: Not a valid branch: lp:REDACTED
10:21:03 E: An error occurred trying to merge lp:donations-service: 'NoneType' object is not iterable
10:21:03 E: Traceback (most recent call last):
10:21:03 E: File "./bin/tarmac", line 8, in <module>
10:21:03 E: sys.exit(main())
10:21:03 E: File "/mnt/tarmac/tarmac/tarmac/bin/__init__.py", line 30, in main
10:21:03 E: return registry.run(args)
10:21:03 E: File "/mnt/tarmac/tarmac/tarmac/bin/registry.py", line 60, in run
10:21:03 E: return self._run(args)
10:21:03 E: File "/mnt/tarmac/tarmac/tarmac/bin/registry.py", line 48, in _run
10:21:03 E: return run_bzr(args)
10:21:03 E: File "/usr/lib/python2.7/dist-packages/bzrlib/commands.py", line 1131, in run_bzr
10:21:03 E: ret = run(*run_argv)
10:21:03 E: File "/usr/lib/python2.7/dist-packages/bzrlib/commands.py", line 673, in run_argv_aliases
10:21:03 E: return self.run(**all_cmd_args)
10:21:03 E: File "/usr/lib/python2.7/dist-packages/bzrlib/commands.py", line 695, in run
10:21:03 E: return self._operation.run_simple(*args, **kwargs)
10:21:03 E: File "/usr/lib/python2.7/dist-packages/bzrlib/cleanup.py", line 136, in run_simple
10:21:03 E: self.cleanups, self.func, *args, **kwargs)
10:21:03 E: File "/usr/lib/python2.7/dist-packages/bzrlib/cleanup.py", line 166, in _do_with_cleanups
10:21:03 E: result = func(*args, **kwargs)
10:21:03 E: File "/mnt/tarmac/tarmac/tarmac/bin/commands.py", line 379, in run
10:21:03 E: statuses.extend(self._do_merges(branch, verify_command=verify_command))
10:21:03 E: TypeError: 'NoneType' object is not iterable
10:21:03 I: Finished with exitcode 1
Build step 'Execute shell' marked build as failure

This reveals three problems:
 1. "Not a valid branch: lp:REDACTED" is only lightly informative. It doesn't help explain why lp:REDACTED is invalid.
 2. Tarmac crashes when it can't find a branch
 3. Our tarmac bot doesn't have access to lp:REDACTED

The third is a configuration issue that we are addressing. The first and second are bugs in tarmac, which this merge proposal fixes.

It addresses the problem by refactoring out some logic from the merge & check commands for loading branches & merge proposals from Launchpad.

It changes the "Not a valid branch: lp:foo" message to "tarmac-bot could not find lp:foo branch on Launchpad".

To post a comment you must log in.
Revision history for this message
Natalia Bidart (nataliabidart) wrote :

(10:41:54 AM) nessita: jml: looks good. No tests in tarmac? :-/
(10:42:47 AM) jml: nessita: tarmac has tests, yes.
(10:43:32 AM) nessita: jml: no test need modification/adding after these changes?
(10:43:32 AM) jml: launchpadlib is a colossal pain in the neck, which discourages me from testing.
(10:45:48 AM) nessita: I can empathize with that, though... perhaps a test to confirm that the log message is the expected can be added?
(10:45:49 AM) jml: nessita: well, the tests still pass :)
(10:45:57 AM) jml: nessita: yeah, I'll try to add something.
(10:46:02 AM) nessita: jml: thanks

lp:~jml/tarmac/handle-private-branch updated
432. By Jonathan Lange

I hate these tests so much.

433. By Jonathan Lange

Test branch not existing.

Revision history for this message
Natalia Bidart (nataliabidart) wrote :

Thanks!

review: Approve

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 2013-01-03 22:03:59 +0000
3+++ tarmac/bin/commands.py 2013-01-14 15:01:21 +0000
4@@ -10,6 +10,7 @@
5 from launchpadlib.launchpad import Launchpad
6 from launchpadlib.uris import (LPNET_SERVICE_ROOT,
7 STAGING_SERVICE_ROOT)
8+from lazr.restfulclient.errors import Unauthorized
9
10 from tarmac.bin import options
11 from tarmac.branch import Branch
12@@ -134,6 +135,20 @@
13 help_commands(self.outf)
14
15
16+def _get_login_name(lp):
17+ """Return the name of the user `lp` is logged in as.
18+
19+ `None` if `lp` is an anonymous connection.
20+ """
21+ try:
22+ me = lp.me
23+ except Unauthorized:
24+ return None
25+ if me:
26+ return me.name
27+ return None
28+
29+
30 def _get_mergable_proposals_for_branch(lp_branch, logger, imply_commit_message=False):
31 """Return a list of the mergable proposals for the given branch."""
32 proposals = []
33@@ -155,6 +170,43 @@
34 return proposals
35
36
37+def _get_branch(branch_url, launchpad, logger):
38+ lp_branch = launchpad.branches.getByUrl(url=branch_url)
39+ if lp_branch is None:
40+ logger.info(
41+ 'User {0} could not find {1} branch on Launchpad'.format(
42+ _get_login_name(launchpad), branch_url))
43+ return lp_branch
44+
45+
46+def _get_branch_and_mps(branch_url, launchpad, logger,
47+ imply_commit_message=False):
48+ """Get branch and merge proposals from Launchpad.
49+
50+ :param branch_url: The Launchpad URL of the branch. e.g `lp:foo`.
51+ :param launchpad: A Launchpad API object.
52+ :param logger: A Python logger.
53+ :param imply_commit_message: Whether to make up a commit message
54+ if the merge proposal lacks an explicit one. Defaults to False.
55+ :return: `(lp_branch, [mergable_mp, ...])`
56+ """
57+ lp_branch = _get_branch(branch_url, launchpad, logger)
58+ proposals = []
59+
60+ if lp_branch:
61+ proposals = _get_mergable_proposals_for_branch(
62+ lp_branch, logger,
63+ imply_commit_message=imply_commit_message)
64+
65+ if not proposals:
66+ logger.info(
67+ 'No approved proposals found for %(branch_url)s' % {
68+ 'branch_url': branch_url})
69+
70+ return lp_branch, proposals
71+
72+
73+
74 def _get_reviews(proposal):
75 """Get the set of reviews from the proposal."""
76 votes = [vote for vote in proposal.votes if vote.comment]
77@@ -324,19 +376,11 @@
78 ]
79
80 def _do_merges(self, branch_url, verify_command=None):
81-
82- lp_branch = self.launchpad.branches.getByUrl(url=branch_url)
83- if lp_branch is None:
84- self.logger.info('Not a valid branch: {0}'.format(branch_url))
85- return
86-
87- proposals = _get_mergable_proposals_for_branch(lp_branch, self.logger,
88- imply_commit_message=self.config.imply_commit_message)
89+ lp_branch, proposals = _get_branch_and_mps(
90+ branch_url, self.launchpad, self.logger,
91+ self.config.imply_commit_message)
92
93 if not proposals:
94- self.logger.info(
95- 'No approved proposals found for %(branch_url)s' % {
96- 'branch_url': branch_url})
97 return []
98
99 if self.config.one:
100@@ -348,7 +392,6 @@
101 statuses = merge_proposals(target, proposals, self.logger, self.config, self)
102 return statuses
103
104-
105 def run(self, branch_url=None, launchpad=None, verify_command=None, **kwargs):
106 for key, value in kwargs.iteritems():
107 self.config.set('Tarmac', key, value)
108@@ -402,20 +445,9 @@
109 options.debug_option]
110
111 def _any_merges(self, branch_url):
112-
113- lp_branch = self.launchpad.branches.getByUrl(url=branch_url)
114- if lp_branch is None:
115- self.logger.info('Not a valid branch: {0}'.format(branch_url))
116- return
117-
118- proposals = _get_mergable_proposals_for_branch(lp_branch, self.logger)
119-
120- if not proposals:
121- self.logger.info(
122- 'No approved proposals found for %(branch_url)s' % {
123- 'branch_url': branch_url})
124- return False
125- return True
126+ lp_branch, proposals = _get_branch_and_mps(
127+ branch_url, self.launchpad, self.logger)
128+ return bool(proposals)
129
130 def run(self, branch_url, launchpad=None, **kwargs):
131 for key, value in kwargs.iteritems():
132
133=== modified file 'tarmac/tests/test_commands.py'
134--- tarmac/tests/test_commands.py 2012-12-20 12:21:44 +0000
135+++ tarmac/tests/test_commands.py 2013-01-14 15:01:21 +0000
136@@ -1,5 +1,6 @@
137 '''Tests for tarmac.bin.commands.py.'''
138 from cStringIO import StringIO
139+import logging
140 import os
141 import shutil
142 import sys
143@@ -152,7 +153,8 @@
144 reviewer=Thing(display_name=u'Reviewer2'))])]
145 self.branches[1].landing_candidates = self.proposals
146
147- self.launchpad = Thing(branches=Thing(getByUrl=self.getBranchByUrl))
148+ self.launchpad = Thing(branches=Thing(getByUrl=self.getBranchByUrl),
149+ me=None)
150 self.error = None
151 registry = CommandRegistry(config=self.config)
152 registry.register_command('merge', commands.cmd_merge)
153@@ -174,6 +176,20 @@
154 except IndexError:
155 return None
156
157+ def test__get_branch_exists(self):
158+ # _get_branch returns the lp_branch if it exists.
159+ branch = commands._get_branch(
160+ self.branch2.lp_branch.bzr_identity,
161+ self.launchpad,
162+ logging.getLogger('tarmac'))
163+ self.assertIs(self.branches[0], branch)
164+
165+ def test__get_branch_doesnt_exist(self):
166+ # _get_branch returns None if it doesn't exist.
167+ branch = commands._get_branch(
168+ 'doesntexist', self.launchpad, logging.getLogger('tarmac'))
169+ self.assertIs(None, branch)
170+
171 def test_run(self):
172 """Test that the merge command merges a branch successfully."""
173 self.proposals[1].reviewed_revid = \

Subscribers

People subscribed via source and target branches