Launchpad itself

Merge lp:~bac/launchpad/gmp into lp:launchpad

  1. gmp
  2. Merge into devel
Proposed by Brad Crittenden
Status: Merged
Merged at revision: not available
Proposed branch: lp:~bac/launchpad/gmp
Merge into: lp:launchpad
Diff against target: 251 lines
3 files modified
lib/devscripts/autoland.py (+58/-7)
lib/devscripts/ec2test/builtins.py (+25/-10)
lib/devscripts/tests/test_autoland.py (+12/-1)
To merge this branch: bzr merge lp:~bac/launchpad/gmp
Reviewer Review Type Date Requested Status
Paul Hummer (community) code Approve
Canonical Launchpad Engineering Pending
Review via email: mp+13083@code.launchpad.net

Commit message

Make 'ec2 land' figure out where to find the merge proposal so that it is optional.

To post a comment you must log in.
Revision history for this message
Brad Crittenden (bac) wrote :

= Summary =

'ec2 land' is a great improvement in submitting branches via ec2 for landing as it
figures out all of the good parts and creates the PQM submit message. The original
version, however, requires the URL to the merge proposal as an argument. There is no
reason for requiring the MP as it can be found by using the public branch or push
location.

== Proposed fix ==

Make the merge proposal optional and if there is exactly one merge proposal for the
branch use it.

With this fix, if a developer is sitting in a branch that has an approved MP, she can
 send it off for test and landing by just doing:

% ec2 land

Presto bingo!

== Pre-implementation notes ==

Talked with Jonathan Lange and got good suggestions on how to make it more robust,
including falling back on the push location.

== Implementation details ==

None really.

== Tests ==

Erm...

== Demo and Q/A ==

Go to the directory for a branch that has a merge proposal in 'Approved' state and try:

% ec2 land --dry-run

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/devscripts/autoland.py
  lib/devscripts/ec2test/builtins.py

== Pylint notices ==

lib/devscripts/autoland.py
    124: [C0301] Line too long (79/78)

lib/devscripts/ec2test/builtins.py
    244: [C0301] Line too long (79/78)
    490: [C0301] Line too long (79/78)
    256: [W0102, cmd_test.run] Dangerous default value [] as argument
    428: [W0102, cmd_demo.run] Dangerous default value [] as argument
    500: [W0102, cmd_update_image.run] Dangerous default value [] as argument

I'll fix these before landing as appropriate.

Revision history for this message
Paul Hummer (rockstar) :
review: Approve (ui)
Revision history for this message
Paul Hummer (rockstar) :
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 2009-09-30 12:34:02 +0000
3+++ lib/devscripts/autoland.py 2009-10-08 23:25:23 +0000
4@@ -5,6 +5,8 @@
5 from launchpadlib.launchpad import (
6 Launchpad, EDGE_SERVICE_ROOT, STAGING_SERVICE_ROOT)
7 from lazr.uri import URI
8+from bzrlib.errors import BzrCommandError
9+
10
11 # XXX: JonathanLange 2009-09-24: Both of these are available in more recent
12 # versions of launchpadlib. When we start using such versions, we should
13@@ -42,6 +44,42 @@
14 web_mp_uri.path.lstrip('/'))
15 return MergeProposal(self._launchpad.load(str(api_mp_uri)))
16
17+ def get_lp_branch(self, branch):
18+ """Get the launchpadlib branch based on a bzr branch."""
19+ # First try the public branch.
20+ branch_url = branch.get_public_branch()
21+ if branch_url:
22+ lp_branch = self._launchpad.branches.getByUrl(
23+ url=branch_url)
24+ if lp_branch is not None:
25+ return lp_branch
26+ # If that didn't work try the push location.
27+ branch_url = branch.get_push_location()
28+ if branch_url:
29+ lp_branch = self._launchpad.branches.getByUrl(
30+ url=branch_url)
31+ if lp_branch is not None:
32+ return lp_branch
33+ raise BzrCommandError(
34+ "No public branch could be found. Please re-run and specify "
35+ "the URL for the merge proposal.")
36+
37+ def get_merge_proposal_from_branch(self, branch):
38+ """Get the merge proposal from the branch."""
39+
40+ lp_branch = self.get_lp_branch(branch)
41+ proposals = lp_branch.landing_targets
42+ if len(proposals) == 0:
43+ raise BzrCommandError(
44+ "The public branch has no source merge proposals. "
45+ "You must have a merge proposal before attempting to "
46+ "land the branch.")
47+ elif len(proposals) > 1:
48+ raise BzrCommandError(
49+ "The public branch has multiple source merge proposals. "
50+ "You must provide the URL to the one you wish to use.")
51+ return MergeProposal(proposals[0])
52+
53
54 class MergeProposal:
55 """Wrapper around launchpadlib `IBranchMergeProposal` for landing."""
56@@ -83,7 +121,8 @@
57 """
58 # XXX: JonathanLange 2009-09-24: No unit tests.
59 return set(
60- map(get_email, [self._mp.source_branch.owner, self._launchpad.me]))
61+ map(get_email,
62+ [self._mp.source_branch.owner, self._launchpad.me]))
63
64 def get_reviews(self):
65 """Return a dictionary of all Approved reviews.
66@@ -177,8 +216,11 @@
67
68
69 def _comma_separated_names(things):
70- """Return a string of comma-separated names of 'things'."""
71- return ','.join(thing.name for thing in things)
72+ """Return a string of comma-separated names of 'things'.
73+
74+ The list is sorted before being joined.
75+ """
76+ return ','.join(sorted(thing.name for thing in things))
77
78
79 def get_reviewer_clause(reviewers):
80@@ -188,17 +230,26 @@
81 returned by 'get_reviews'.
82 :return: A string like u'[r=foo,bar][ui=plop]'.
83 """
84+ # If no review type is specified it will be None and is assumed to be a
85+ # code review.
86 code_reviewers = reviewers.get(None, [])
87- code_reviewers.extend(reviewers.get('code', []))
88- code_reviewers.extend(reviewers.get('db', []))
89+ ui_reviewers = []
90+ rc_reviewers = []
91+ for review_type, reviewer in reviewers.items():
92+ if review_type is None:
93+ continue
94+ if 'code' in review_type or 'db' in review_type:
95+ code_reviewers.extend(reviewer)
96+ if 'ui' in review_type:
97+ ui_reviewers.extend(reviewer)
98+ if 'release-critical' in review_type:
99+ rc_reviewers.extend(reviewer)
100 if not code_reviewers:
101 raise MissingReviewError("Need approved votes in order to land.")
102- ui_reviewers = reviewers.get('ui', [])
103 if ui_reviewers:
104 ui_clause = _comma_separated_names(ui_reviewers)
105 else:
106 ui_clause = 'none'
107- rc_reviewers = reviewers.get('release-critical', [])
108 if rc_reviewers:
109 rc_clause = (
110 '[release-critical=%s]' % _comma_separated_names(rc_reviewers))
111
112=== modified file 'lib/devscripts/ec2test/builtins.py'
113--- lib/devscripts/ec2test/builtins.py 2009-10-05 13:31:40 +0000
114+++ lib/devscripts/ec2test/builtins.py 2009-10-08 23:25:23 +0000
115@@ -10,6 +10,7 @@
116 import pdb
117 import subprocess
118
119+from bzrlib.bzrdir import BzrDir
120 from bzrlib.commands import Command
121 from bzrlib.errors import BzrCommandError
122 from bzrlib.help import help_commands
123@@ -240,8 +241,8 @@
124 postmortem_option,
125 Option(
126 'headless',
127- help=('After building the instance and test, run the remote tests '
128- 'headless. Cannot be used with postmortem '
129+ help=('After building the instance and test, run the remote '
130+ 'tests headless. Cannot be used with postmortem '
131 'or file.')),
132 debug_option,
133 Option(
134@@ -252,7 +253,7 @@
135
136 takes_args = ['test_branch?']
137
138- def run(self, test_branch=None, branch=[], trunk=False, machine=None,
139+ def run(self, test_branch=None, branch=None, trunk=False, machine=None,
140 instance_type=DEFAULT_INSTANCE_TYPE,
141 file=None, email=None, test_options='-vv', noemail=False,
142 submit_pqm_message=None, pqm_public_location=None,
143@@ -261,6 +262,8 @@
144 include_download_cache_changes=False):
145 if debug:
146 pdb.set_trace()
147+ if branch is None:
148+ branch = []
149 branches, test_branch = _get_branches_and_test_branch(
150 trunk, branch, test_branch)
151 if ((postmortem or file) and headless):
152@@ -321,7 +324,7 @@
153 help="Land the branch even if the proposal is not approved."),
154 ]
155
156- takes_args = ['merge_proposal']
157+ takes_args = ['merge_proposal?']
158
159 def _get_landing_command(self, source_url, target_url, commit_message,
160 emails):
161@@ -338,7 +341,7 @@
162 commit_message, str(source_url)])
163 return command
164
165- def run(self, merge_proposal, machine=None,
166+ def run(self, merge_proposal=None, machine=None,
167 instance_type=DEFAULT_INSTANCE_TYPE, postmortem=False,
168 debug=False, commit_text=None, dry_run=False, testfix=False,
169 print_commit=False, force=False):
170@@ -361,7 +364,13 @@
171 raise BzrCommandError(
172 "Cannot specify --print-commit and --dry-run.")
173 lander = LaunchpadBranchLander.load()
174- mp = lander.load_merge_proposal(merge_proposal)
175+
176+ if merge_proposal is None:
177+ (tree, bzrbranch, relpath) = (
178+ BzrDir.open_containing_tree_or_branch('.'))
179+ mp = lander.get_merge_proposal_from_branch(bzrbranch)
180+ else:
181+ mp = lander.load_merge_proposal(merge_proposal)
182 if not mp.is_approved:
183 if force:
184 print "Merge proposal is not approved, landing anyway."
185@@ -418,11 +427,13 @@
186
187 takes_args = ['test_branch?']
188
189- def run(self, test_branch=None, branch=[], trunk=False, machine=None,
190+ def run(self, test_branch=None, branch=None, trunk=False, machine=None,
191 instance_type=DEFAULT_INSTANCE_TYPE, debug=False,
192 include_download_cache_changes=False, demo=None):
193 if debug:
194 pdb.set_trace()
195+ if branch is None:
196+ branch = []
197 branches, test_branch = _get_branches_and_test_branch(
198 trunk, branch, test_branch)
199
200@@ -480,8 +491,9 @@
201 ListOption(
202 'extra-update-image-command', type=str,
203 help=('Run this command (with an ssh agent) on the image before '
204- 'running the default update steps. Can be passed more than '
205- 'once, the commands will be run in the order specified.')),
206+ 'running the default update steps. Can be passed more '
207+ 'than once, the commands will be run in the order '
208+ 'specified.')),
209 Option(
210 'public',
211 help=('Remove proprietary code from the sourcecode directory '
212@@ -491,11 +503,14 @@
213 takes_args = ['ami_name']
214
215 def run(self, ami_name, machine=None, instance_type='m1.large',
216- debug=False, postmortem=False, extra_update_image_command=[],
217+ debug=False, postmortem=False, extra_update_image_command=None,
218 public=False):
219 if debug:
220 pdb.set_trace()
221
222+ if extra_update_image_command is None:
223+ extra_update_image_command = []
224+
225 credentials = EC2Credentials.load_from_file()
226
227 instance = EC2Instance.make(
228
229=== modified file 'lib/devscripts/tests/test_autoland.py'
230--- lib/devscripts/tests/test_autoland.py 2009-09-24 11:23:24 +0000
231+++ lib/devscripts/tests/test_autoland.py 2009-10-08 23:25:23 +0000
232@@ -114,7 +114,18 @@
233 # Branches can have more than one reviewer.
234 clause = self.get_reviewer_clause(
235 {None: [self.makePerson('foo'), self.makePerson('bar')]})
236- self.assertEqual('[r=foo,bar][ui=none]', clause)
237+ self.assertEqual('[r=bar,foo][ui=none]', clause)
238+
239+ def test_mentat_reviewers(self):
240+ # A mentat review sometimes is marked like 'ui*'. Due to the
241+ # unordered nature of dictionaries, the reviewers are sorted before
242+ # being put into the clause for predictability.
243+ clause = self.get_reviewer_clause(
244+ {None: [self.makePerson('foo')],
245+ 'code*': [self.makePerson('newguy')],
246+ 'ui': [self.makePerson('beuno')],
247+ 'ui*': [self.makePerson('bac')]})
248+ self.assertEqual('[r=foo,newguy][ui=bac,beuno]', clause)
249
250 def test_code_reviewer_counts(self):
251 # Some people explicitly specify the 'code' type when they do code