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_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 > + """ Kudos for documenting the parameters. It's a bit surprising though to see you set the parameters in __init__, and then optionally override them here. Is the "double interface" pulling its weight? Or would things be better if you just passed the same mail_from/mail_to to every to_email call without having them in the object? > + 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): There should be exactly 2 blank lines above this function definition. Normally "make lint" would complain about this, but unfortunately it won't if you've already committed the change and there are changes in other files that have not yet been committed. > === 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.')), Is the "-address" in the option name meaningful? Would there be any risk of confusion if you just called this "merge-proposal"? Or, assuming the address is a URL, maybe "url" would be clearer than "address." Separately from that, the indentation seems a bit inconsistent here: Option( "indented by one tab", help=("Not indented " "but continued on tab-indented lines.")) I'd do that as: Option( "indented by one tab", help=( "Not indented " "but continued on tab-indented lines.")) > @@ -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."), Another question, because "naming things" is one of the famous two things in software engineering that are hard(*): which would be better, "use-tarmac" or just "tarmac"? The "use" doesn't seem to convey a lot of information. I honestly don't know the right answer but I like short option names. ☺ > @@ -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) The parameter list is getting a bit long for positional arguments. You may want to start using keyword syntax in calls — although that last parameter's name is a bit long to spell out twice. > === 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 > @@ -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. Very nice to see this behaviour documented explicitly. > @@ -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) Program any C or C++ lately? Please fix. ☺ > @@ -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) When continuing "if" conditions on the next line, it becomes a bit hard to see where the condition stops and the body begins. That can slow down people reading through the code. Could you separate them in some way? One standard way to do that is to introduce a variable. Another is to insert a comment between the condition and the code: if (self._request._success_message is None and self._request._failure_message is None): # Don't vote on the MP; go straight to PQM. self._handle_pqm_submission(successful) > + else: > + self._request.send_mergeproposal_email(successful) This confuses me. The name "_handle_pqm_submission" seems to imply that a PQM submission has occurred, and it needs to be handled. But the "if" condition appears completely unrelated to that. I would guess that: 1. the presence of _success_message and/or _failure_message implies something about what action is being requested; and 2. _handle_pqm_submission actually just means "submit to PQM," and the word "handle" is just a wrapper to turn "submission" into a verb. If guess 1 is correct, please make the meaning you attach to _success_message and/or _failure_message being set explicit. I'll offer a more detailed suggestions for that below. If guess 2 is correct, please see if _handle_pqm_submission can be renamed to something like _submit_pqm. > @@ -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 ' Typo: *an* email. > + '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.')) Probably the most immediately important piece of information here is "this is of interest to you if and only if you're working on the ec2 script." How about stating "for use by 'ec2 land' only" right at the beginning? Then, you're trying to cram an explanation of the entire procedure into a single-sentence description. Can you find a good way to break it down into shorter sentences? > + 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.')) Similar, but is "rejected" really the correct resulting status? Why not "needs fixing"? > @@ -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 This repeats both the implicit meaning of success_message/failure_message, and the multi-line "if" condition. Perhaps this should be a method or property. Its name would help clarify its meaning, and you'd have a single-line "if" again. Also, I wonder: the documentation for the --success-message and --failure-message options doesn't say what happens if you only set --success-message but the tests fail, or if you only set --failure-message but the tests pass. Centralizing that choice in one place would make it easier for a reader to recognize, and also to change if desired. > + 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')) A lot of identical compound actions there. Maybe extract the repetition into a function, e.g. "unpickle_message"? > @@ -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) This is definitely too long to continue using positional arguments, especially with pqm_message changing to a different parameter position! > === 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 > @@ -102,6 +105,57 @@ > return dict(map(normalize_branch_input, branches)) > > > +class MergeProposalMessage(EmailMessage): A touch of déjà vu here! Is there no way to share 1 version of this class between autoland.py and testrunner.py? > @@ -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 To prevent misunderstandings about how the options will interact, would it make sense to raise an error if mergepropsoal_address and pqm_message were both set? > @@ -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 A good place for a blank line, I agree. But two is a bit much. ☺ > @@ -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') There's the multi-line "if" again. To make it easier to follow the code's intention, it may be worthwhile to introduce a helpfully-named Boolean variable. For instance, if that's what the condition means, you might call it "may_use_email." > === 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 > @@ -65,6 +65,43 @@ > pass > > > +class FakeGPGStrategy: > + > + def __init__(self, text_to_return=""): > + self.sign = FakeMethod(text_to_return) > + > + > +class TestMergeProposalMessage(unittest.TestCase): Careful! You've got 2 MergeProposalMessage classes, but you're only testing one. > + def setUp(self): > + self.mail_to = "