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
=== modified file 'lib/devscripts/autoland.py'
--- lib/devscripts/autoland.py 2009-09-30 12:34:02 +0000
+++ lib/devscripts/autoland.py 2009-10-08 23:25:23 +0000
@@ -5,6 +5,8 @@
5from launchpadlib.launchpad import (5from launchpadlib.launchpad import (
6 Launchpad, EDGE_SERVICE_ROOT, STAGING_SERVICE_ROOT)6 Launchpad, EDGE_SERVICE_ROOT, STAGING_SERVICE_ROOT)
7from lazr.uri import URI7from lazr.uri import URI
8from bzrlib.errors import BzrCommandError
9
810
9# XXX: JonathanLange 2009-09-24: Both of these are available in more recent11# XXX: JonathanLange 2009-09-24: Both of these are available in more recent
10# versions of launchpadlib. When we start using such versions, we should12# versions of launchpadlib. When we start using such versions, we should
@@ -42,6 +44,42 @@
42 web_mp_uri.path.lstrip('/'))44 web_mp_uri.path.lstrip('/'))
43 return MergeProposal(self._launchpad.load(str(api_mp_uri)))45 return MergeProposal(self._launchpad.load(str(api_mp_uri)))
4446
47 def get_lp_branch(self, branch):
48 """Get the launchpadlib branch based on a bzr branch."""
49 # First try the public branch.
50 branch_url = branch.get_public_branch()
51 if branch_url:
52 lp_branch = self._launchpad.branches.getByUrl(
53 url=branch_url)
54 if lp_branch is not None:
55 return lp_branch
56 # If that didn't work try the push location.
57 branch_url = branch.get_push_location()
58 if branch_url:
59 lp_branch = self._launchpad.branches.getByUrl(
60 url=branch_url)
61 if lp_branch is not None:
62 return lp_branch
63 raise BzrCommandError(
64 "No public branch could be found. Please re-run and specify "
65 "the URL for the merge proposal.")
66
67 def get_merge_proposal_from_branch(self, branch):
68 """Get the merge proposal from the branch."""
69
70 lp_branch = self.get_lp_branch(branch)
71 proposals = lp_branch.landing_targets
72 if len(proposals) == 0:
73 raise BzrCommandError(
74 "The public branch has no source merge proposals. "
75 "You must have a merge proposal before attempting to "
76 "land the branch.")
77 elif len(proposals) > 1:
78 raise BzrCommandError(
79 "The public branch has multiple source merge proposals. "
80 "You must provide the URL to the one you wish to use.")
81 return MergeProposal(proposals[0])
82
4583
46class MergeProposal:84class MergeProposal:
47 """Wrapper around launchpadlib `IBranchMergeProposal` for landing."""85 """Wrapper around launchpadlib `IBranchMergeProposal` for landing."""
@@ -83,7 +121,8 @@
83 """121 """
84 # XXX: JonathanLange 2009-09-24: No unit tests.122 # XXX: JonathanLange 2009-09-24: No unit tests.
85 return set(123 return set(
86 map(get_email, [self._mp.source_branch.owner, self._launchpad.me]))124 map(get_email,
125 [self._mp.source_branch.owner, self._launchpad.me]))
87126
88 def get_reviews(self):127 def get_reviews(self):
89 """Return a dictionary of all Approved reviews.128 """Return a dictionary of all Approved reviews.
@@ -177,8 +216,11 @@
177216
178217
179def _comma_separated_names(things):218def _comma_separated_names(things):
180 """Return a string of comma-separated names of 'things'."""219 """Return a string of comma-separated names of 'things'.
181 return ','.join(thing.name for thing in things)220
221 The list is sorted before being joined.
222 """
223 return ','.join(sorted(thing.name for thing in things))
182224
183225
184def get_reviewer_clause(reviewers):226def get_reviewer_clause(reviewers):
@@ -188,17 +230,26 @@
188 returned by 'get_reviews'.230 returned by 'get_reviews'.
189 :return: A string like u'[r=foo,bar][ui=plop]'.231 :return: A string like u'[r=foo,bar][ui=plop]'.
190 """232 """
233 # If no review type is specified it will be None and is assumed to be a
234 # code review.
191 code_reviewers = reviewers.get(None, [])235 code_reviewers = reviewers.get(None, [])
192 code_reviewers.extend(reviewers.get('code', []))236 ui_reviewers = []
193 code_reviewers.extend(reviewers.get('db', []))237 rc_reviewers = []
238 for review_type, reviewer in reviewers.items():
239 if review_type is None:
240 continue
241 if 'code' in review_type or 'db' in review_type:
242 code_reviewers.extend(reviewer)
243 if 'ui' in review_type:
244 ui_reviewers.extend(reviewer)
245 if 'release-critical' in review_type:
246 rc_reviewers.extend(reviewer)
194 if not code_reviewers:247 if not code_reviewers:
195 raise MissingReviewError("Need approved votes in order to land.")248 raise MissingReviewError("Need approved votes in order to land.")
196 ui_reviewers = reviewers.get('ui', [])
197 if ui_reviewers:249 if ui_reviewers:
198 ui_clause = _comma_separated_names(ui_reviewers)250 ui_clause = _comma_separated_names(ui_reviewers)
199 else:251 else:
200 ui_clause = 'none'252 ui_clause = 'none'
201 rc_reviewers = reviewers.get('release-critical', [])
202 if rc_reviewers:253 if rc_reviewers:
203 rc_clause = (254 rc_clause = (
204 '[release-critical=%s]' % _comma_separated_names(rc_reviewers))255 '[release-critical=%s]' % _comma_separated_names(rc_reviewers))
205256
=== modified file 'lib/devscripts/ec2test/builtins.py'
--- lib/devscripts/ec2test/builtins.py 2009-10-05 13:31:40 +0000
+++ lib/devscripts/ec2test/builtins.py 2009-10-08 23:25:23 +0000
@@ -10,6 +10,7 @@
10import pdb10import pdb
11import subprocess11import subprocess
1212
13from bzrlib.bzrdir import BzrDir
13from bzrlib.commands import Command14from bzrlib.commands import Command
14from bzrlib.errors import BzrCommandError15from bzrlib.errors import BzrCommandError
15from bzrlib.help import help_commands16from bzrlib.help import help_commands
@@ -240,8 +241,8 @@
240 postmortem_option,241 postmortem_option,
241 Option(242 Option(
242 'headless',243 'headless',
243 help=('After building the instance and test, run the remote tests '244 help=('After building the instance and test, run the remote '
244 'headless. Cannot be used with postmortem '245 'tests headless. Cannot be used with postmortem '
245 'or file.')),246 'or file.')),
246 debug_option,247 debug_option,
247 Option(248 Option(
@@ -252,7 +253,7 @@
252253
253 takes_args = ['test_branch?']254 takes_args = ['test_branch?']
254255
255 def run(self, test_branch=None, branch=[], trunk=False, machine=None,256 def run(self, test_branch=None, branch=None, trunk=False, machine=None,
256 instance_type=DEFAULT_INSTANCE_TYPE,257 instance_type=DEFAULT_INSTANCE_TYPE,
257 file=None, email=None, test_options='-vv', noemail=False,258 file=None, email=None, test_options='-vv', noemail=False,
258 submit_pqm_message=None, pqm_public_location=None,259 submit_pqm_message=None, pqm_public_location=None,
@@ -261,6 +262,8 @@
261 include_download_cache_changes=False):262 include_download_cache_changes=False):
262 if debug:263 if debug:
263 pdb.set_trace()264 pdb.set_trace()
265 if branch is None:
266 branch = []
264 branches, test_branch = _get_branches_and_test_branch(267 branches, test_branch = _get_branches_and_test_branch(
265 trunk, branch, test_branch)268 trunk, branch, test_branch)
266 if ((postmortem or file) and headless):269 if ((postmortem or file) and headless):
@@ -321,7 +324,7 @@
321 help="Land the branch even if the proposal is not approved."),324 help="Land the branch even if the proposal is not approved."),
322 ]325 ]
323326
324 takes_args = ['merge_proposal']327 takes_args = ['merge_proposal?']
325328
326 def _get_landing_command(self, source_url, target_url, commit_message,329 def _get_landing_command(self, source_url, target_url, commit_message,
327 emails):330 emails):
@@ -338,7 +341,7 @@
338 commit_message, str(source_url)])341 commit_message, str(source_url)])
339 return command342 return command
340343
341 def run(self, merge_proposal, machine=None,344 def run(self, merge_proposal=None, machine=None,
342 instance_type=DEFAULT_INSTANCE_TYPE, postmortem=False,345 instance_type=DEFAULT_INSTANCE_TYPE, postmortem=False,
343 debug=False, commit_text=None, dry_run=False, testfix=False,346 debug=False, commit_text=None, dry_run=False, testfix=False,
344 print_commit=False, force=False):347 print_commit=False, force=False):
@@ -361,7 +364,13 @@
361 raise BzrCommandError(364 raise BzrCommandError(
362 "Cannot specify --print-commit and --dry-run.")365 "Cannot specify --print-commit and --dry-run.")
363 lander = LaunchpadBranchLander.load()366 lander = LaunchpadBranchLander.load()
364 mp = lander.load_merge_proposal(merge_proposal)367
368 if merge_proposal is None:
369 (tree, bzrbranch, relpath) = (
370 BzrDir.open_containing_tree_or_branch('.'))
371 mp = lander.get_merge_proposal_from_branch(bzrbranch)
372 else:
373 mp = lander.load_merge_proposal(merge_proposal)
365 if not mp.is_approved:374 if not mp.is_approved:
366 if force:375 if force:
367 print "Merge proposal is not approved, landing anyway."376 print "Merge proposal is not approved, landing anyway."
@@ -418,11 +427,13 @@
418427
419 takes_args = ['test_branch?']428 takes_args = ['test_branch?']
420429
421 def run(self, test_branch=None, branch=[], trunk=False, machine=None,430 def run(self, test_branch=None, branch=None, trunk=False, machine=None,
422 instance_type=DEFAULT_INSTANCE_TYPE, debug=False,431 instance_type=DEFAULT_INSTANCE_TYPE, debug=False,
423 include_download_cache_changes=False, demo=None):432 include_download_cache_changes=False, demo=None):
424 if debug:433 if debug:
425 pdb.set_trace()434 pdb.set_trace()
435 if branch is None:
436 branch = []
426 branches, test_branch = _get_branches_and_test_branch(437 branches, test_branch = _get_branches_and_test_branch(
427 trunk, branch, test_branch)438 trunk, branch, test_branch)
428439
@@ -480,8 +491,9 @@
480 ListOption(491 ListOption(
481 'extra-update-image-command', type=str,492 'extra-update-image-command', type=str,
482 help=('Run this command (with an ssh agent) on the image before '493 help=('Run this command (with an ssh agent) on the image before '
483 'running the default update steps. Can be passed more than '494 'running the default update steps. Can be passed more '
484 'once, the commands will be run in the order specified.')),495 'than once, the commands will be run in the order '
496 'specified.')),
485 Option(497 Option(
486 'public',498 'public',
487 help=('Remove proprietary code from the sourcecode directory '499 help=('Remove proprietary code from the sourcecode directory '
@@ -491,11 +503,14 @@
491 takes_args = ['ami_name']503 takes_args = ['ami_name']
492504
493 def run(self, ami_name, machine=None, instance_type='m1.large',505 def run(self, ami_name, machine=None, instance_type='m1.large',
494 debug=False, postmortem=False, extra_update_image_command=[],506 debug=False, postmortem=False, extra_update_image_command=None,
495 public=False):507 public=False):
496 if debug:508 if debug:
497 pdb.set_trace()509 pdb.set_trace()
498510
511 if extra_update_image_command is None:
512 extra_update_image_command = []
513
499 credentials = EC2Credentials.load_from_file()514 credentials = EC2Credentials.load_from_file()
500515
501 instance = EC2Instance.make(516 instance = EC2Instance.make(
502517
=== modified file 'lib/devscripts/tests/test_autoland.py'
--- lib/devscripts/tests/test_autoland.py 2009-09-24 11:23:24 +0000
+++ lib/devscripts/tests/test_autoland.py 2009-10-08 23:25:23 +0000
@@ -114,7 +114,18 @@
114 # Branches can have more than one reviewer.114 # Branches can have more than one reviewer.
115 clause = self.get_reviewer_clause(115 clause = self.get_reviewer_clause(
116 {None: [self.makePerson('foo'), self.makePerson('bar')]})116 {None: [self.makePerson('foo'), self.makePerson('bar')]})
117 self.assertEqual('[r=foo,bar][ui=none]', clause)117 self.assertEqual('[r=bar,foo][ui=none]', clause)
118
119 def test_mentat_reviewers(self):
120 # A mentat review sometimes is marked like 'ui*'. Due to the
121 # unordered nature of dictionaries, the reviewers are sorted before
122 # being put into the clause for predictability.
123 clause = self.get_reviewer_clause(
124 {None: [self.makePerson('foo')],
125 'code*': [self.makePerson('newguy')],
126 'ui': [self.makePerson('beuno')],
127 'ui*': [self.makePerson('bac')]})
128 self.assertEqual('[r=foo,newguy][ui=bac,beuno]', clause)
118129
119 def test_code_reviewer_counts(self):130 def test_code_reviewer_counts(self):
120 # Some people explicitly specify the 'code' type when they do code131 # Some people explicitly specify the 'code' type when they do code