Merge lp:~ursinha/launchpad/auto-approve into lp:launchpad

[Ursula Junque]

These changes implement ec2 land capability to work with the new Launchpad Tarmac merge workflow.

= Summary =

This branch adds the -T (--use-tarmac) option to ec2 land, so when the tests finish, one "tests" vote is added to the merge proposal, and its queue status is set to approved or rejected.
For this to work, I had to add a --mergeproposal-address to ec2 test, considering ec2 land is just a wrapper that generates an ec2 test command line and runs it as a subprocess.

== Tests ==

I've tested the newly created method by adding more tests to test_autoland.py.

./bin/test -vvt devscripts.*

== Demo and Q/A ==

I've tested by running the command in a real merge proposal in Launchpad, using the --dry-run option, to check if the command line is correctly generated by ec2 land.

$ ec2 land <merge_proposal> --force --dry-run --use-tarmac

Should show the command line with the ec2 test --mergeproposal-address I've added so it can generate the signed message to send launchpad after the tests.

[Jeroen T. Vermeulen (jtv)]

So now we're getting "tests" votes? Nice! That's something I've been wanting.

The branch looks good overall, though it wouldn't be me if there weren't some comments.

Some of those are things that need changing. Most of them are mere suggestions. I trust you to get the changes right, so I'm giving you an approval vote. But feel free to ask if you do want them reviewed, of course.

> === modified file 'lib/devscripts/autoland.py'
> --- lib/devscripts/autoland.py 2011-01-04 20:22:39 +0000
> +++ lib/devscripts/autoland.py 2011-01-11 13:08:59 +0000
> @@ -6,6 +6,8 @@
> STAGING_SERVICE_ROOT)
> from lazr.uri import URI
> from bzrlib.errors import BzrCommandError
> +from bzrlib.email_message import EmailMessage
> +from bzrlib.config import GlobalConfig

Those should be in alphabetical order. Which is a drag to keep up. Run utilities/format-new-and-modified-imports to automate the drudgery.

> @@ -191,6 +193,63 @@
> self._mp.lp_save()
>
>
> +class MergeProposalMessage(EmailMessage):
> +
> + def __init__(self, mail_to, mail_from, successful=None,
> + source_branch=None, gpg_strategy=None):

Our coding standards say the continuation line(s) for the parameters in the method definition (not in calls mind you!) should be indented to the same column as the first parameter.

So the "source_branch" parameter aligns with "self." If this comes through correctly:

    def __init__(self, mail_to, mail_from, successful=None,
                 source_branch=None, gpg_strategy=None):

> + if successful:
> + self.subject = "Test results: SUCCESS"
> + self.body_text = """
> +Full test results were mailed to the submitter.
> +
> + review approve tests
> + merge approved
> +"""
> + else:
> + self.subject = "Test results: FAILURE"
> + self.body_text = """
> +Full test results were mailed to the submitter.
> +
> + review needsfixing tests
> + merge rejected
> +"""

Getting rid of unwanted indentation in multi-line strings is always a pain. Dedent() alone is not enough because the string starts out with a newline: thus not every line begins with indentation. Personally I consider it a bug but we have to live with it.

One question though: those two body texts are identical in structure and similar in text. How significant is that? If it's something inherent, it may help to abstract away the differences. For example:

    if successful:
        test_outcome = "SUCCESS"
        vote = "approve"
        status = "approved"
    else:
        test_outcome = "FAILURE"
        vote = "needsfixing"
        status = "rejected"

    self.subject = "Test results: %s" % test_outcome
    self.body_text = '\n'.join([
        "Full test results were mailed to the submitter.",
        "",
        "review %s tests" % vote,
        "merge %s" % status,
        ])

Mind you, just an example. Another approach might be to create an email template in a separate file, as in lib/lp/*/emailtemplates. But it hardly seems worth the trouble.

> + self.successful = successful
> + self.source_branch = source_branch
> + self.mail_from = mail_from
> + self.mail_to = mail_...

review: Approve (code)

[Robert Collins (lifeless)]

Its my understanding that tarmac is on hold - we're unlikely to even get close to it in 2011; to avoid having an unused codepath I'm going to suggest we don't land this at this point - I'm putting this proposal into 'WIP' status until we get back to working on our landing story.

Unmerged revisions

12054. By Ursula Junque on 2011-01-10

Added tests to MergeProposalMessage class

12053. By Ursula Junque on 2011-01-10

moved MergeProposalMessage to autoland.

12052. By Ursula Junque on 2011-01-10

moved MergeProposalMessage to autoland.py, where MergeProposal is defined.

12051. By Ursula Junque on 2011-01-10

Merging latest devel

12050. By Ursula Junque on 2011-01-07

fixed spacing

12049. By Ursula Junque on 2011-01-07

change parameters when calling Request

12048. By Ursula Junque on 2011-01-07

Added a MergeProposalMessage class to represent a signed message to be sent to the MP, changing it according to the test results.

12047. By Ursula Junque on 2011-01-07

removing the signing phase of merge proposal messages, now it gets both preassembled success/failure messages and decodes as it does with pqm_message

12046. By Ursula Junque on 2011-01-05

updated tests and standarized variable and methods names

12045. By Ursula Junque on 2011-01-05

adding workaround to ask the user signature before tests start, to after that email the results to the launchpad mp. It also adds that if mergeproposal address is provided, pqm submission is ignored.

=== modified file 'lib/devscripts/autoland.py'
--- lib/devscripts/autoland.py	2011-01-04 20:22:39 +0000
+++ lib/devscripts/autoland.py	2011-01-11 13:08:59 +0000
@@ -6,6 +6,8 @@
     STAGING_SERVICE_ROOT)
 from lazr.uri import URI
 from bzrlib.errors import BzrCommandError
+from bzrlib.email_message import EmailMessage
+from bzrlib.config import GlobalConfig
 
 
 class MissingReviewError(Exception):
@@ -191,6 +193,63 @@
         self._mp.lp_save()
 
 
+class MergeProposalMessage(EmailMessage):
+
+    def __init__(self, mail_to, mail_from, successful=None,
+        source_branch=None, gpg_strategy=None):
+
+        if successful:
+            self.subject = "Test results: SUCCESS"
+            self.body_text = """
+Full test results were mailed to the submitter.
+
+ review approve tests
+ merge approved
+"""
+        else:
+            self.subject = "Test results: FAILURE"
+            self.body_text = """
+Full test results were mailed to the submitter.
+
+ review needsfixing tests
+ merge rejected
+"""
+        self.successful = successful
+        self.source_branch = source_branch
+        self.mail_from = mail_from
+        self.mail_to = mail_to
+        self.gpg_strategy = gpg_strategy
+
+    def to_signed(self, unsigned_text):
+        """Serialize as a signed string."""
+
+        if self.source_branch:
+            config = self.source_branch.get_config()
+        else:
+            config = GlobalConfig()
+        if self.gpg_strategy is None:
+            strategy = GPGStrategy(config)
+        else:
+            strategy = self.gpg_strategy
+        return strategy.sign(unsigned_text)
+
+    def to_email(self, mail_from=None, mail_to=None):
+        """Serialize as an email message.
+
+        :param mail_from: The from address for the message
+        :param mail_to: The address to send the message to
+        :return: an email message
+        """
+        if mail_from is None:
+            mail_from = self.mail_from
+        if mail_to is None:
+            mail_to = self.mail_to
+        body = self.to_signed(self.body_text)
+        message = EmailMessage(mail_from, mail_to, self.subject, body)
+        return message
+
+
+
 def get_testfix_clause(testfix=False):
     """Get the testfix clause."""
     if testfix:

=== modified file 'lib/devscripts/ec2test/builtins.py'
--- lib/devscripts/ec2test/builtins.py	2010-11-29 21:32:06 +0000
+++ lib/devscripts/ec2test/builtins.py	2011-01-11 13:08:59 +0000
@@ -223,6 +223,12 @@
                   "to ``-o '-vv'``.  For instance, to run specific tests, "
                   "you might use ``-o '-vvt my_test_pattern'``.")),
         Option(
+            'mergeproposal-address', type=str,
+            help=('Address of the merge proposal to be updated with test'
+                'results. Should be used only by ec2 land. If provided, you '
+                'will be asked for your GPG passphrase before the test run '
+                'begins.')),
+        Option(
             'submit-pqm-message', short_name='s', type=str, argname="MSG",
             help=(
                 'A pqm message to submit if the test run is successful.  If '
@@ -271,7 +277,7 @@
             noemail=False, submit_pqm_message=None, pqm_public_location=None,
             pqm_submit_location=None, pqm_email=None, postmortem=False,
             attached=False, debug=False, open_browser=False,
-            include_download_cache_changes=False):
+            include_download_cache_changes=False, mergeproposal_address=None):
         if debug:
             pdb.set_trace()
         if branch is None:
@@ -312,7 +318,7 @@
             open_browser=open_browser, pqm_email=pqm_email,
             include_download_cache_changes=include_download_cache_changes,
             instance=instance, launchpad_login=instance._launchpad_login,
-            timeout=480)
+            timeout=480, mergeproposal_address=mergeproposal_address)
 
         instance.set_up_and_run(postmortem, attached, runner.run_tests)
 
@@ -324,6 +330,8 @@
         debug_option,
         Option('dry-run', help="Just print the equivalent ec2 test command."),
         Option('print-commit', help="Print the full commit message."),
+        Option('use-tarmac', short_name='T', help="This should be prepared "
+            "for Tarmac landing instead of PQM."),
         Option(
             'testfix',
             help="This is a testfix (tags commit with [testfix])."),
@@ -352,7 +360,7 @@
     takes_args = ['merge_proposal?']
 
     def _get_landing_command(self, source_url, target_url, commit_message,
-                             emails, attached):
+                             emails, attached, mergeproposal_address=None):
         """Return the command that would need to be run to submit with ec2."""
         ec2_path = os.path.join(get_launchpad_root(), 'utilities', 'ec2')
         command = [ec2_path, 'test']
@@ -366,13 +374,16 @@
         command.extend(
             ['-b', 'launchpad=%s' % (target_branch_name), '-s',
              commit_message, str(source_url)])
+        if mergeproposal_address:
+            command.extend(
+                ['--mergeproposal-address=%s' % mergeproposal_address])
         return command
 
     def run(self, merge_proposal=None, machine=None,
             instance_type=DEFAULT_INSTANCE_TYPE, postmortem=False,
             debug=False, commit_text=None, dry_run=False, testfix=False,
             no_qa=False, incremental=False, rollback=None, print_commit=False,
-            force=False, attached=False):
+            force=False, attached=False, use_tarmac=False):
         try:
             from devscripts.autoland import (
                 LaunchpadBranchLander, MissingReviewError, MissingBugsError,
@@ -447,9 +458,14 @@
             print commit_message
             return
 
+        if use_tarmac:
+            mergeproposal_address= mp._mp.address
+        else:
+            mergeproposal_address = None
+
         landing_command = self._get_landing_command(
             mp.source_branch, mp.target_branch, commit_message,
-            mp.get_stakeholder_emails(), attached)
+            mp.get_stakeholder_emails(), attached, mergeproposal_address)
         if dry_run:
             print landing_command
         else:

=== modified file 'lib/devscripts/ec2test/remote.py'
--- lib/devscripts/ec2test/remote.py	2010-10-26 15:47:24 +0000
+++ lib/devscripts/ec2test/remote.py	2011-01-11 13:08:59 +0000
@@ -306,7 +306,8 @@
     """A request to have a branch tested and maybe landed."""
 
     def __init__(self, branch_url, revno, local_branch_path, sourcecode_path,
-                 emails=None, pqm_message=None, smtp_connection=None):
+                 emails=None, success_message=None, failure_message=None,
+                 pqm_message=None, smtp_connection=None):
         """Construct a `Request`.
 
         :param branch_url: The public URL to the Launchpad branch we are
@@ -320,6 +321,12 @@
             dependencies.
         :param emails: A list of emails to send the results to. If not
             provided, no emails are sent.
+        :param success_message: The message to update merge proposal in
+            Launchpad with successful results. If not provided, we don't
+            change the merge proposal.
+        :param failure_message: The message to update merge proposal in
+            Launchpad with failure results. If not provided, we don't
+            change the merge proposal.
         :param pqm_message: The message to submit to PQM. If not provided, we
             don't submit to PQM.
         :param smtp_connection: The `SMTPConnection` to use to send email.
@@ -330,6 +337,8 @@
         self._sourcecode_path = sourcecode_path
         self._emails = emails
         self._pqm_message = pqm_message
+        self._success_message = success_message
+        self._failure_message = failure_message
         # Used for figuring out how to send emails.
         self._bzr_config = bzrlib.config.GlobalConfig()
         if smtp_connection is None:
@@ -502,6 +511,18 @@
         message = self._build_report_email(successful, body_text, full_log_gz)
         self._send_email(message)
 
+    def send_mergeproposal_email(self, successful):
+        """Send an email to the merge proposal adding a tests vote, and
+        approving or disapproving it, according to test results.
+
+        :param successful: True for pass, False for failure.
+        """
+        if successful:
+            message = self._success_message
+        else:
+            message = self._failure_message
+        self._send_email(message)
+
     def iter_dependency_branches(self):
         """Iterate through the Bazaar branches we depend on."""
         for name in sorted(os.listdir(self._sourcecode_path)):
@@ -596,6 +617,8 @@
             summary = self.get_summary_contents()
             full_log_gz = gzip_data(self.get_full_log_contents())
             self._request.send_report_email(False, summary, full_log_gz)
+        if (self._request._failure_message is not None):
+            self._request.send_mergeproposal_email(False)
 
     def get_index_contents(self):
         """Return the contents of the index.html page."""
@@ -632,7 +655,11 @@
         """The tests are done and the results are known."""
         self._end_time = datetime.datetime.utcnow()
         successful = result.wasSuccessful()
-        self._handle_pqm_submission(successful)
+        if (self._request._success_message is None and
+            self._request._failure_message is None):
+            self._handle_pqm_submission(successful)
+        else:
+            self._request.send_mergeproposal_email(successful)
         if self._request.wants_email:
             email_text = self._request.format_result(
                 result, self._start_time, self._end_time)
@@ -846,6 +873,22 @@
               'the email address from `bzr whoami`. May be supplied multiple '
               'times. `bzr whoami` will be used as the From: address.'))
     parser.add_option(
+        '--success-message', dest='success_message', default=None,
+        help=('A base64-encoded pickle (string) of a email message to be '
+            'sent to Launchpad, updating the merge proposal by adding a '
+            'tests vote with an approval, and setting the merge proposal '
+            'status to approved. Should be used only by ec2 land. If '
+            'provided, you will be asked for your GPG passphrase before '
+            'the test run begins.'))
+    parser.add_option(
+        '--failure-message', dest='failure_message', default=None,
+        help=('A base64-encoded pickle (string) of a email message to be '
+            'sent to Launchpad, updating the merge proposal by adding a '
+            'tests vote with a needsfixing, and setting the merge proposal '
+            'status to rejected. Should be used only by ec2 land. If '
+            'provided, you will be asked for your GPG passphrase before '
+            'the test run begins.'))
+    parser.add_option(
         '-s', '--submit-pqm-message', dest='pqm_message', default=None,
         help=('A base64-encoded pickle (string) of a pqm message '
               '(bzrib.plugins.pqm.pqm_submit.PQMEmailMessage) to submit if '
@@ -877,11 +920,20 @@
 
     if options.debug:
         import pdb; pdb.set_trace()
-    if options.pqm_message is not None:
+    if (options.success_message is not None and
+        options.failure_message is not None):
+        pqm_message = None
+        success_message = pickle.loads(
+            options.success_message.decode('string-escape').decode('base64'))
+        failure_message = pickle.loads(
+            options.failure_message.decode('string-escape').decode('base64'))
+    elif options.pqm_message is not None:
         pqm_message = pickle.loads(
             options.pqm_message.decode('string-escape').decode('base64'))
+        success_message = failure_message = None
     else:
         pqm_message = None
+        success_message = failure_message = None
 
     # Locations for Launchpad. These are actually defined by the configuration
     # of the EC2 image that we use.
@@ -895,7 +947,8 @@
 
     request = Request(
         options.public_branch, options.public_branch_revno, TEST_DIR,
-        SOURCECODE_DIR, options.email, pqm_message, smtp_connection)
+        SOURCECODE_DIR, options.email, success_message, failure_message,
+        pqm_message, smtp_connection)
     # Only write to stdout if we are running as the foreground process.
     echo_to_stdout = not options.daemon
     logger = WebTestLogger.make_in_directory(

=== modified file 'lib/devscripts/ec2test/testrunner.py'
--- lib/devscripts/ec2test/testrunner.py	2010-08-18 17:40:29 +0000
+++ lib/devscripts/ec2test/testrunner.py	2011-01-11 13:08:59 +0000
@@ -18,6 +18,9 @@
 from bzrlib.bzrdir import BzrDir
 from bzrlib.config import GlobalConfig
 from bzrlib.errors import UncommittedChanges
+from bzrlib.gpg import GPGStrategy
+from bzrlib.email_message import EmailMessage
+from devscripts.autoland import MergeProposalMessage
 from bzrlib.plugins.pqm.pqm_submit import (
     NoPQMSubmissionAddress, PQMSubmission)
 
@@ -102,6 +105,57 @@
     return dict(map(normalize_branch_input, branches))
 
 
+class MergeProposalMessage(EmailMessage):
+
+    def __init__(self, mail_to, mail_from, successful=None,
+        source_branch=None):
+
+        if successful:
+            self.subject = "Test results: SUCCESS"
+            self.body_text = """
+Full test results were mailed to the submitter.
+
+ review approve tests
+ merge approved
+"""
+        else:
+            self.subject = "Test results: FAILURE"
+            self.body_text = """
+Full test results were mailed to the submitter.
+
+ review needsfixing tests
+ merge rejected
+"""
+        self.source_branch = source_branch
+        self.mail_from = mail_from
+        self.mail_to = mail_to
+
+    def to_signed(self, unsigned_text):
+        """Serialize as a signed string."""
+
+        if self.source_branch:
+            config = self.source_branch.get_config()
+        else:
+            config = GlobalConfig()
+        strategy = GPGStrategy(config)
+        return strategy.sign(unsigned_text)
+
+    def to_email(self, mail_from=None, mail_to=None):
+        """Serialize as an email message.
+
+        :param mail_from: The from address for the message
+        :param mail_to: The address to send the message to
+        :return: an email message
+        """
+        if mail_from is None:
+            mail_from = self.mail_from
+        if mail_to is None:
+            mail_to = self.mail_to
+        body = self.to_signed(self.body_text)
+        message = EmailMessage(mail_from, mail_to, self.subject, body)
+        return message
+
+
 class EC2TestRunner:
 
     name = 'ec2-test-runner'
@@ -116,7 +170,7 @@
                  open_browser=False, pqm_email=None,
                  include_download_cache_changes=None, instance=None,
                  launchpad_login=None,
-                 timeout=None):
+                 timeout=None, mergeproposal_address=None):
         """Create a new EC2TestRunner.
 
         :param timeout: Number of minutes before we force a shutdown. This is
@@ -143,6 +197,7 @@
         self.file = file
         self._launchpad_login = launchpad_login
         self.timeout = timeout
+        self.mergeproposal_address = mergeproposal_address
 
         trunk_specified = False
         trunk_branch = TRUNK_BRANCH
@@ -199,6 +254,13 @@
                         pqm_submit_location = trunk_branch
                 elif pqm_submit_location is None and trunk_specified:
                     pqm_submit_location = trunk_branch
+
+                # Explicitly ignoring pqm submission if the merge proposal
+                # address is provided. That means that we should use tarmac
+                # instead of pqm.
+                if self.mergeproposal_address is not None:
+                    pqm_message = None
+
                 # modified from pqm_submit.py
                 submission = PQMSubmission(
                     source_branch=bzrbranch,
@@ -236,7 +298,20 @@
                                 cache_basis_tree.unlock()
                         finally:
                             cache_tree.unlock()
-                if pqm_message is not None:
+
+                if mergeproposal_address is not None:
+                    self.message = None
+                    self.success_message = MergeProposalMessage(
+                            mergeproposal_address,
+                            config.username(),
+                            successful=True,
+                            source_branch=bzrbranch).to_email()
+                    self.failure_message = MergeProposalMessage(
+                            mergeproposal_address,
+                            config.username(),
+                            successful=False,
+                            source_branch=bzrbranch).to_email()
+                elif pqm_message is not None:
                     if self.download_cache_additions:
                         raise UncommittedChanges(cache_tree)
                     # get the submission message
@@ -255,6 +330,7 @@
                         raise NoPQMSubmissionAddress(bzrbranch)
                     mail_to = pqm_email.encode('utf8') # same here
                     self.message = submission.to_email(mail_from, mail_to)
+                    self.success_message = self.failure_message = None
                 elif (self.download_cache_additions and
                       self.include_download_cache_changes is None):
                     raise UncommittedChanges(
@@ -264,6 +340,8 @@
                         '--ignore-download-cache-changes, -c and -g '
                         'respectively), or '
                         'commit or remove the files in the download-cache.')
+
+
         self._branch = branch
 
         if email is not False:
@@ -287,7 +365,8 @@
         self.email = email
 
         # Email configuration.
-        if email is not None or pqm_message is not None:
+        if (email is not None or pqm_message is not None or
+            mergeproposal_address is not None):
             self._smtp_server = config.get_user_option('smtp_server')
             # Refuse localhost, because there's no SMTP server _on the actual
             # EC2 instance._

=== modified file 'lib/devscripts/ec2test/tests/test_remote.py'
--- lib/devscripts/ec2test/tests/test_remote.py	2010-10-06 11:46:51 +0000
+++ lib/devscripts/ec2test/tests/test_remote.py	2011-01-11 13:08:59 +0000
@@ -74,7 +74,8 @@
 
     def make_request(self, branch_url=None, revno=None,
                      trunk=None, sourcecode_path=None,
-                     emails=None, pqm_message=None, emails_sent=None):
+                     emails=None, success_message=None, failure_message=None,
+                     pqm_message=None, emails_sent=None):
         """Make a request to test, specifying only things we care about.
 
         Note that the returned request object will not ever send email, but
@@ -93,7 +94,7 @@
         smtp_connection = LoggingSMTPConnection(emails_sent)
         request = Request(
             branch_url, revno, trunk.basedir, sourcecode_path, emails,
-            pqm_message, smtp_connection)
+            success_message, failure_message, pqm_message, smtp_connection)
         return request
 
     def make_sourcecode(self, branches):

=== modified file 'lib/devscripts/tests/test_autoland.py'
--- lib/devscripts/tests/test_autoland.py	2010-11-05 18:48:39 +0000
+++ lib/devscripts/tests/test_autoland.py	2011-01-11 13:08:59 +0000
@@ -16,7 +16,7 @@
     get_bazaar_host, get_bugs_clause, get_reviewer_clause,
     get_reviewer_handle, get_qa_clause, get_testfix_clause,
     MissingReviewError, MissingBugsError, MissingBugsIncrementalError,
-    MergeProposal)
+    MergeProposal, MergeProposalMessage)
 
 
 class FakeBug:
@@ -65,6 +65,43 @@
         pass
 
 
+class FakeGPGStrategy:
+
+    def __init__(self, text_to_return=""):
+        self.sign = FakeMethod(text_to_return)
+
+
+class TestMergeProposalMessage(unittest.TestCase):
+
+    def setUp(self):
+        self.mail_to = "[e-mail]"
+        self.mail_from = "[e-mail]"
+        self.gpg_strategy = FakeGPGStrategy()
+
+    def test_merge_proposal_message_is_successful(self):
+        successful = True
+        message = MergeProposalMessage(self.mail_to, self.mail_from,
+            successful=successful, gpg_strategy=self.gpg_strategy)
+        self.assertTrue(message.successful)
+        self.assertTrue("SUCCESS" in message.subject)
+
+    def test_merge_proposal_message_not_successful(self):
+        successful = False
+        message = MergeProposalMessage(self.mail_to, self.mail_from,
+            successful=successful, gpg_strategy=self.gpg_strategy)
+        self.assertFalse(message.successful)
+        self.assertTrue("FAILURE" in message.subject)
+
+    def test_to_email(self):
+        fake_signed_text = "signed msg body"
+        gpg_strategy = FakeGPGStrategy(fake_signed_text)
+
+        message = MergeProposalMessage(self.mail_to, self.mail_from,
+            successful=True, gpg_strategy=gpg_strategy)
+        message = message.to_email()
+        self.assertEqual(message._body, fake_signed_text)
+
+
 class TestPQMRegexAcceptance(unittest.TestCase):
     """Tests if the generated commit message is accepted by PQM regexes."""