Merge lp:~jml/launchpad/various-ec2-fixes into lp:launchpad

[Jonathan Lange]

[r=jcsackett][bug=439348,493774,540016,607434,638468,732029] Many usability improvements to 'ec2 test' and 'ec2 land'.

A whole bunch of fixes for ec2 test. I can't really add much here that's not in the bug reports. The ec2test system isn't super well-tested in general, and I haven't tried very hard to fix that in this branch. I have verified that ec2 update-image and ec2 land still work, that we trust the merge proposal reviewer and that we exclude merge proposals that are approved. I have not verified the EINTR handling (it's tricky) or the bug status fix.

[j.c.sackett (jcsackett)]

This looks like a good grabbag of fixes.

review: Approve

=== modified file 'lib/devscripts/autoland.py'
--- lib/devscripts/autoland.py	2011-02-01 00:43:23 +0000
+++ lib/devscripts/autoland.py	2011-03-31 16:08:32 +0000
@@ -67,16 +67,19 @@
         """Get the merge proposal from the branch."""
 
         lp_branch = self.get_lp_branch(branch)
-        proposals = lp_branch.landing_targets
+        proposals = [
+            mp for mp in lp_branch.landing_targets
+            if mp.queue_status in ('Needs review', 'Approved')]
         if len(proposals) == 0:
             raise BzrCommandError(
-                "The public branch has no source merge proposals.  "
+                "The public branch has no open source merge proposals.  "
                 "You must have a merge proposal before attempting to "
                 "land the branch.")
         elif len(proposals) > 1:
             raise BzrCommandError(
-                "The public branch has multiple source merge proposals.  "
-                "You must provide the URL to the one you wish to use.")
+                "The public branch has multiple open source merge "
+                "proposals.  You must provide the URL to the one you wish "
+                "to use.")
         return MergeProposal(proposals[0])
 
 
@@ -139,6 +142,8 @@
                 continue
             reviewers = reviews.setdefault(vote.review_type, [])
             reviewers.append(vote.reviewer)
+        if self.is_approved and not reviews:
+            reviews[None] = [self._mp.reviewer]
         return reviews
 
     def get_bugs(self):
@@ -249,7 +254,16 @@
     """
     if not bugs:
         return ''
-    return '[bug=%s]' % ','.join(str(bug.id) for bug in bugs)
+    bug_ids = []
+    for bug in bugs:
+        for task in bug.bug_tasks:
+            if (task.bug_target_name == 'launchpad'
+                and task.status not in ['Fix Committed', 'Fix Released']):
+                bug_ids.append(str(bug.id))
+                break
+    if not bug_ids:
+        return ''
+    return '[bug=%s]' % ','.join(bug_ids)
 
 
 def get_reviewer_handle(reviewer):

=== modified file 'lib/devscripts/ec2test/builtins.py'
--- lib/devscripts/ec2test/builtins.py	2011-02-24 13:47:13 +0000
+++ lib/devscripts/ec2test/builtins.py	2011-03-31 16:08:32 +0000
@@ -10,7 +10,6 @@
 import os
 import pdb
 import socket
-import subprocess
 
 from bzrlib.bzrdir import BzrDir
 from bzrlib.commands import Command
@@ -314,8 +313,7 @@
                 "supported")
 
         session_name = EC2SessionName.make(EC2TestRunner.name)
-        instance = EC2Instance.make(
-            session_name, instance_type, machine)
+        instance = EC2Instance.make(session_name, instance_type, machine)
 
         runner = EC2TestRunner(
             test_branch, email=email, file=file,
@@ -460,15 +458,33 @@
             print commit_message
             return
 
+        emails = mp.get_stakeholder_emails()
+
+        target_branch_name = mp.target_branch.split('/')[-1]
+        branches = [('launchpad', target_branch_name)]
+
         landing_command = self._get_landing_command(
             mp.source_branch, mp.target_branch, commit_message,
-            mp.get_stakeholder_emails(), attached)
+            emails, attached)
+
         if dry_run:
             print landing_command
-        else:
-            # XXX: JonathanLange 2009-09-24 bug=439348: Call EC2 APIs
-            # directly, rather than spawning a subprocess.
-            return subprocess.call(landing_command)
+            return
+
+        session_name = EC2SessionName.make(EC2TestRunner.name)
+        instance = EC2Instance.make(
+            session_name, instance_type, machine)
+
+        runner = EC2TestRunner(
+            mp.source_branch, email=emails,
+            headless=(not attached),
+            branches=branches, pqm_message=commit_message,
+            instance=instance,
+            launchpad_login=instance._launchpad_login,
+            test_options=DEFAULT_TEST_OPTIONS,
+            timeout=480)
+
+        instance.set_up_and_run(postmortem, attached, runner.run_tests)
 
 
 class cmd_demo(EC2Command):
@@ -616,6 +632,8 @@
         """
         # Do NOT accept environment variables via ssh connections.
         user_connection = instance.connect()
+        user_connection.perform('sudo apt-get -qqy update')
+        user_connection.perform('sudo apt-get -qqy upgrade')
         user_connection.perform(
             'sudo sed -i "s/^AcceptEnv/#AcceptEnv/" /etc/ssh/sshd_config')
         user_connection.perform(

=== modified file 'lib/devscripts/ec2test/instance.py'
--- lib/devscripts/ec2test/instance.py	2010-11-23 17:26:02 +0000
+++ lib/devscripts/ec2test/instance.py	2011-03-31 16:08:32 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Code to represent a single machine instance in EC2."""
@@ -9,6 +9,7 @@
     ]
 
 import code
+import errno
 import glob
 import os
 import select
@@ -622,7 +623,11 @@
         session.exec_command(cmd)
         session.shutdown_write()
         while 1:
-            select.select([session], [], [], 0.5)
+            try:
+                select.select([session], [], [], 0.5)
+            except (IOError, select.error), e:
+                if e.errno == errno.EINTR:
+                    continue
             if session.recv_ready():
                 data = session.recv(4096)
                 if data:

=== modified file 'lib/devscripts/ec2test/tests/test_remote.py'
--- lib/devscripts/ec2test/tests/test_remote.py	2011-02-28 00:54:12 +0000
+++ lib/devscripts/ec2test/tests/test_remote.py	2011-03-31 16:08:32 +0000
@@ -540,7 +540,9 @@
         [body, attachment] = email.get_payload()
         self.assertIsInstance(body, MIMEText)
         self.assertEqual('inline', body['Content-Disposition'])
-        self.assertEqual('text/plain; charset="utf-8"', body['Content-Type'])
+        self.assertIn(
+            body['Content-Type'],
+            ['text/plain; charset="utf-8"', 'text/plain; charset="utf8"'])
         self.assertEqual("foo", body.get_payload(decode=True))
 
     def test_report_email_attachment(self):
@@ -820,7 +822,9 @@
         [body, attachment] = email.get_payload()
         self.assertIsInstance(body, MIMEText)
         self.assertEqual('inline', body['Content-Disposition'])
-        self.assertEqual('text/plain; charset="utf-8"', body['Content-Type'])
+        self.assertIn(
+            body['Content-Type'],
+            ['text/plain; charset="utf-8"', 'text/plain; charset="utf8"'])
         self.assertEqual(
             logger.get_summary_contents(), body.get_payload(decode=True))
         self.assertIsInstance(attachment, MIMEApplication)

=== modified file 'lib/devscripts/tests/test_autoland.py'
--- lib/devscripts/tests/test_autoland.py	2011-02-01 00:43:23 +0000
+++ lib/devscripts/tests/test_autoland.py	2011-03-31 16:08:32 +0000
@@ -19,14 +19,24 @@
     MergeProposal)
 
 
+class FakeBugTask:
+
+    def __init__(self, target_name, status):
+        self.bug_target_name = target_name
+        self.status = status
+
+
 class FakeBug:
     """Fake launchpadlib Bug object.
 
     Only used for the purposes of testing.
     """
 
-    def __init__(self, id):
+    def __init__(self, id, bug_tasks=None):
         self.id = id
+        if bug_tasks is None:
+            bug_tasks = [FakeBugTask('launchpad', 'Triaged')]
+        self.bug_tasks = bug_tasks
 
 
 class FakePerson:
@@ -113,7 +123,7 @@
         self._test_commit_message_match(incr=True, no_qa=False, testfix=False)
 
 
-class TestBugsClaused(unittest.TestCase):
+class TestBugsClause(unittest.TestCase):
     """Tests for `get_bugs_clause`."""
 
     def test_no_bugs(self):
@@ -134,6 +144,36 @@
         bugs_clause = get_bugs_clause([bug1, bug2])
         self.assertEqual('[bug=20,45]', bugs_clause)
 
+    def test_fixed_bugs_are_excluded(self):
+        # If a bug is fixed then it is excluded from the bugs clause.
+        bug1 = FakeBug(20)
+        bug2 = FakeBug(45, bug_tasks=[
+            FakeBugTask('fake-project', 'Fix Released')])
+        bug3 = FakeBug(67, bug_tasks=[
+            FakeBugTask('fake-project', 'Fix Committed')])
+        bugs_clause = get_bugs_clause([bug1, bug2, bug3])
+        self.assertEqual('[bug=20]', bugs_clause)
+
+    def test_bugs_open_on_launchpad_are_included(self):
+        # If a bug has been fixed on one target but not in launchpad, then it
+        # is included in the bugs clause, because it's relevant to launchpad
+        # QA.
+        bug = FakeBug(20, bug_tasks=[
+            FakeBugTask('fake-project', 'Fix Released'),
+            FakeBugTask('launchpad', 'Triaged')])
+        bugs_clause = get_bugs_clause([bug])
+        self.assertEqual('[bug=20]', bugs_clause)
+
+    def test_bugs_fixed_on_launchpad_but_open_in_others_are_excluded(self):
+        # If a bug has been fixed in Launchpad but not fixed on a different
+        # target, then it is excluded from the bugs clause, since we don't
+        # want to QA it.
+        bug = FakeBug(20, bug_tasks=[
+            FakeBugTask('fake-project', 'Triaged'),
+            FakeBugTask('launchpad', 'Fix Released')])
+        bugs_clause = get_bugs_clause([bug])
+        self.assertEqual('', bugs_clause)
+
 
 class TestGetCommitMessage(unittest.TestCase):
 
@@ -229,7 +269,7 @@
 
         self.assertEqual(
             "[r=foo][bug=20][no-qa][incr] Foobaring the sbrubble.",
-            self.mp.build_commit_message("Foobaring the sbrubble.", 
+            self.mp.build_commit_message("Foobaring the sbrubble.",
                 testfix, no_qa, incr))
 
     def test_commit_with_rollback(self):
@@ -334,10 +374,7 @@
 
     def test_rollback_and_noqa_and_incr_given(self):
         bugs = None
-        no_qa = True
-        incr = True
-        self.assertEqual('[rollback=123]',
-            get_qa_clause(bugs, rollback=123))
+        self.assertEqual('[rollback=123]', get_qa_clause(bugs, rollback=123))
 
 
 class TestGetReviewerHandle(unittest.TestCase):