Merge lp:~jml/launchpad/ec2-muck-around into lp:launchpad

[Jonathan Lange]

Clean up the ec2 tools a bit, removing the vals dict and increasing PEP 8 compliance.

[Jonathan Lange (jml)]

I got bored today and started hacking on this branch. It doesn't really add any features or change any behaviour (I hope), but it cleans up the ec2 tools a little. Most of the work is focused around the EC2TestRunner constructor, which I was trying to understand.

Here's what I did specifically:

  * More PEP 8 compliance: 80 cols, whitespace etc.
  * Cleanup of spurious comments
  * Move some option checking to the builtins module and out of the constructor
  * Drop all uses of the 'vals' dict.

I haven't really tested it, since I'm not sure how.

jml

[Michael Hudson-Doyle (mwhudson)]

Hi,

> I got bored today and started hacking on this branch. It doesn't really add
> any features or change any behaviour (I hope), but it cleans up the ec2 tools
> a little. Most of the work is focused around the EC2TestRunner constructor,
> which I was trying to understand.
>
> Here's what I did specifically:
>
> * More PEP 8 compliance: 80 cols, whitespace etc.
> * Cleanup of spurious comments
> * Move some option checking to the builtins module and out of the
> constructor
> * Drop all uses of the 'vals' dict.

This all sounds great, particularly all the line lengths that I was too lazy to fix.

> I haven't really tested it, since I'm not sure how.

You just need to run all the commands I think :/ Landing it via ec2 test -s you can test one of them at least :-)

Cheers,
mwh

review: Approve

=== modified file 'lib/devscripts/ec2test/builtins.py'
--- lib/devscripts/ec2test/builtins.py	2009-10-03 08:19:54 +0000
+++ lib/devscripts/ec2test/builtins.py	2009-10-04 16:05:20 +0000
@@ -108,6 +108,32 @@
           'and/or of this script.'))
 
 
+def filename_type(filename):
+    """An option validator for filenames.
+
+    :raise: an error if 'filename' is not a file we can write to.
+    :return: 'filename' otherwise.
+    """
+    if filename is None:
+        return filename
+
+    check_file = filename
+    if os.path.exists(check_file):
+        if not os.path.isfile(check_file):
+            raise BzrCommandError(
+                'file argument %s exists and is not a file' % (filename,))
+    else:
+        check_file = os.path.dirname(check_file)
+        if (not os.path.exists(check_file) or
+            not os.path.isdir(check_file)):
+            raise BzrCommandError(
+                'file %s cannot be created.' % (filename,))
+    if not os.access(check_file, os.W_OK):
+        raise BzrCommandError(
+            'you do not have permission to write %s' % (filename,))
+    return filename
+
+
 class EC2Command(Command):
     """Subclass of `Command` that customizes usage to say 'ec2' not 'bzr'.
 
@@ -164,51 +190,54 @@
         machine_id_option,
         instance_type_option,
         Option(
-            'file', short_name='f',
+            'file', short_name='f', type=filename_type,
             help=('Store abridged test results in FILE.')),
         ListOption(
             'email', short_name='e', argname='EMAIL', type=str,
-            help=('Email address to which results should be mailed.  Defaults to '
-                  'the email address from `bzr whoami`. May be supplied multiple '
-                  'times. The first supplied email address will be used as the '
-                  'From: address.')),
+            help=('Email address to which results should be mailed.  '
+                  'Defaults to the email address from `bzr whoami`. May be '
+                  'supplied multiple times. The first supplied email address '
+                  'will be used as the From: address.')),
         Option(
             'noemail', short_name='n',
             help=('Do not try to email results.')),
         Option(
             'test-options', short_name='o', type=str,
-            help=('Test options to pass to the remote test runner.  Defaults to '
-                  "``-o '-vv'``.  For instance, to run specific tests, you might "
-                  "use ``-o '-vvt my_test_pattern'``.")),
+            help=('Test options to pass to the remote test runner.  Defaults '
+                  "to ``-o '-vv'``.  For instance, to run specific tests, "
+                  "you might use ``-o '-vvt my_test_pattern'``.")),
         Option(
             'submit-pqm-message', short_name='s', type=str, argname="MSG",
-            help=('A pqm message to submit if the test run is successful.  If '
-                  'provided, you will be asked for your GPG passphrase before '
-                  'the test run begins.')),
+            help=(
+                'A pqm message to submit if the test run is successful.  If '
+                'provided, you will be asked for your GPG passphrase before '
+                'the test run begins.')),
         Option(
             'pqm-public-location', type=str,
-            help=('The public location for the pqm submit, if a pqm message is '
-                  'provided (see --submit-pqm-message).  If this is not provided, '
-                  'for local branches, bzr configuration is consulted; for '
-                  'remote branches, it is assumed that the remote branch *is* '
-                  'a public branch.')),
+            help=('The public location for the pqm submit, if a pqm message '
+                  'is provided (see --submit-pqm-message).  If this is not '
+                  'provided, for local branches, bzr configuration is '
+                  'consulted; for remote branches, it is assumed that the '
+                  'remote branch *is* a public branch.')),
         Option(
             'pqm-submit-location', type=str,
-            help=('The submit location for the pqm submit, if a pqm message is '
-                  'provided (see --submit-pqm-message).  If this option is not '
-                  'provided, the script will look for an explicitly specified '
-                  'launchpad branch using the -b/--branch option; if that branch '
-                  'was specified and is owned by the launchpad-pqm user on '
-                  'launchpad, it is used as the pqm submit location. Otherwise, '
-                  'for local branches, bzr configuration is consulted; for '
-                  'remote branches, it is assumed that the submit branch is %s.'
+            help=('The submit location for the pqm submit, if a pqm message '
+                  'is provided (see --submit-pqm-message).  If this option '
+                  'is not provided, the script will look for an explicitly '
+                  'specified launchpad branch using the -b/--branch option; '
+                  'if that branch was specified and is owned by the '
+                  'launchpad-pqm user on launchpad, it is used as the pqm '
+                  'submit location. Otherwise, for local branches, bzr '
+                  'configuration is consulted; for remote branches, it is '
+                  'assumed that the submit branch is %s.'
                   % (TRUNK_BRANCH,))),
         Option(
             'pqm-email', type=str,
-            help=('Specify the email address of the PQM you are submitting to. '
-                  'If the branch is local, then the bzr configuration is '
-                  'consulted; for remote branches "Launchpad PQM '
-                  '<[e-mail]>" is used by default.')),
+            help=(
+                'Specify the email address of the PQM you are submitting to. '
+                'If the branch is local, then the bzr configuration is '
+                'consulted; for remote branches "Launchpad PQM '
+                '<[e-mail]>" is used by default.')),
         postmortem_option,
         Option(
             'headless',
@@ -252,6 +281,10 @@
                 'You have specified no way to get the results '
                 'of your headless test run.')
 
+        if test_options != '-vv' and submit_pqm_message is not None:
+            raise BzrCommandError(
+                "Submitting to PQM with non-default test options isn't "
+                "supported")
 
         instance = EC2Instance.make(
             EC2TestRunner.name, instance_type, machine)
@@ -264,7 +297,7 @@
             pqm_submit_location=pqm_submit_location,
             open_browser=open_browser, pqm_email=pqm_email,
             include_download_cache_changes=include_download_cache_changes,
-            instance=instance, vals=instance._vals)
+            instance=instance, launchpad_login=instance._launchpad_login)
 
         instance.set_up_and_run(postmortem, not headless, runner.run_tests)
 
@@ -387,7 +420,7 @@
         runner = EC2TestRunner(
             test_branch, branches=branches,
             include_download_cache_changes=include_download_cache_changes,
-            instance=instance, vals=instance._vals)
+            instance=instance)
 
         demo_network_string = '\n'.join(
             '  ' + network for network in demo)
@@ -483,13 +516,15 @@
             directory before bundling.
         """
         user_connection = instance.connect()
-        user_connection.perform('bzr launchpad-login %(launchpad-login)s')
+        user_connection.perform(
+            'bzr launchpad-login %s' % (instance._launchpad_login,))
         for cmd in extra_update_image_command:
             user_connection.run_with_ssh_agent(cmd)
         user_connection.run_with_ssh_agent(
             'bzr pull -d /var/launchpad/test ' + TRUNK_BRANCH)
         user_connection.run_with_ssh_agent(
-            'bzr pull -d /var/launchpad/download-cache lp:lp-source-dependencies')
+            'bzr pull -d /var/launchpad/download-cache '
+            'lp:lp-source-dependencies')
         if public:
             update_sourcecode_options = '--public-only'
         else:

=== modified file 'lib/devscripts/ec2test/instance.py'
--- lib/devscripts/ec2test/instance.py	2009-10-03 08:19:54 +0000
+++ lib/devscripts/ec2test/instance.py	2009-10-04 16:05:20 +0000
@@ -29,13 +29,14 @@
 DEFAULT_INSTANCE_TYPE = 'c1.xlarge'
 AVAILABLE_INSTANCE_TYPES = ('m1.large', 'm1.xlarge', 'c1.xlarge')
 
+
 class AcceptAllPolicy:
     """We accept all unknown host key."""
 
-    # Normally the console output is supposed to contain the Host key
-    # but it doesn't seem to be the case here, so we trust that the host
-    # we are connecting to is the correct one.
     def missing_host_key(self, client, hostname, key):
+        # Normally the console output is supposed to contain the Host key but
+        # it doesn't seem to be the case here, so we trust that the host we
+        # are connecting to is the correct one.
         pass
 
 
@@ -210,28 +211,26 @@
         # get the image
         image = account.acquire_image(machine_id)
 
-        vals = os.environ.copy()
         login = get_lp_login()
         if not login:
             raise BzrCommandError(
                 'you must have set your launchpad login in bzr.')
-        vals['launchpad-login'] = login
 
         return EC2Instance(
-            name, image, instance_type, demo_networks, account, vals,
-            from_scratch, user_key)
+            name, image, instance_type, demo_networks, account,
+            from_scratch, user_key, login)
 
     def __init__(self, name, image, instance_type, demo_networks, account,
-                 vals, from_scratch, user_key):
+                 from_scratch, user_key, launchpad_login):
         self._name = name
         self._image = image
         self._account = account
         self._instance_type = instance_type
         self._demo_networks = demo_networks
         self._boto_instance = None
-        self._vals = vals
         self._from_scratch = from_scratch
         self._user_key = user_key
+        self._launchpad_login = launchpad_login
 
     def log(self, msg):
         """Log a message on stdout, flushing afterwards."""
@@ -320,7 +319,8 @@
         """
         authorized_keys_file = conn.sftp.open(remote_filename, 'w')
         authorized_keys_file.write(
-            "%s %s\n" % (self._user_key.get_name(), self._user_key.get_base64()))
+            "%s %s\n" % (
+                self._user_key.get_name(), self._user_key.get_base64()))
         authorized_keys_file.close()
 
     def _ensure_ec2test_user_has_keys(self, connection=None):
@@ -361,11 +361,13 @@
             self._upload_local_key(root_connection, 'local_key')
             root_connection.perform(
                 'cat local_key >> ~/.ssh/authorized_keys && rm local_key')
-            root_connection.run_script(from_scratch_root % self._vals)
+            root_connection.run_script(from_scratch_root)
             self._ensure_ec2test_user_has_keys(root_connection)
             root_connection.close()
             conn = self._connect('ec2test')
-            conn.run_script(from_scratch_ec2test % self._vals)
+            conn.run_script(
+                from_scratch_ec2test
+                % {'launchpad-login': self._launchpad_login})
             self._from_scratch = False
             return conn
         self._ensure_ec2test_user_has_keys()
@@ -390,9 +392,10 @@
             try:
                 return func(*args, **kw)
             except Exception:
-                # When running in postmortem mode, it is really helpful to see if
-                # there are any exceptions before it waits in the console (in the
-                # finally block), and you can't figure out why it's broken.
+                # When running in postmortem mode, it is really helpful to see
+                # if there are any exceptions before it waits in the console
+                # (in the finally block), and you can't figure out why it's
+                # broken.
                 traceback.print_exc()
         finally:
             try:
@@ -400,7 +403,8 @@
                     console = code.InteractiveConsole(locals())
                     console.interact((
                         'Postmortem Console.  EC2 instance is not yet dead.\n'
-                        'It will shut down when you exit this prompt (CTRL-D).\n'
+                        'It will shut down when you exit this prompt '
+                        '(CTRL-D).\n'
                         '\n'
                         'Tab-completion is enabled.'
                         '\n'
@@ -486,7 +490,7 @@
         connection.perform('sudo mkdir /mnt/ec2')
         connection.perform('sudo chown $USER:$USER /mnt/ec2')
 
-        remote_pk, remote_cert =  self.copy_key_and_certificate_to_image(
+        remote_pk, remote_cert = self.copy_key_and_certificate_to_image(
             connection.sftp)
 
         bundle_dir = os.path.join('/mnt', name)
@@ -530,7 +534,7 @@
             'ec2-register',
             '--private-key=%s' % self.local_pk,
             '--cert=%s' % self.local_cert,
-            manifest_path
+            manifest_path,
             ]
         self.log("Executing command: %s" % ' '.join(cmd))
         subprocess.check_call(cmd, env=env)
@@ -551,14 +555,18 @@
             self._sftp = self._ssh.open_sftp()
         return self._sftp
 
-    def perform(self, cmd, ignore_failure=False, out=None):
+    def perform(self, cmd, ignore_failure=False, out=None, err=None):
         """Perform 'cmd' on server.
 
         :param ignore_failure: If False, raise an error on non-zero exit
             statuses.
         :param out: A stream to write the output of the remote command to.
+        :param err: A stream to write the error of the remote command to.
         """
-        cmd = cmd % self._instance._vals
+        if out is None:
+            out = sys.stdout
+        if err is None:
+            err = sys.stderr
         self._instance.log(
             '%s@%s$ %s\n'
             % (self._username, self._instance._boto_instance.id, cmd))
@@ -570,15 +578,13 @@
             if session.recv_ready():
                 data = session.recv(4096)
                 if data:
-                    sys.stdout.write(data)
-                    sys.stdout.flush()
-                    if out is not None:
-                        out.write(data)
+                    out.write(data)
+                    out.flush()
             if session.recv_stderr_ready():
                 data = session.recv_stderr(4096)
                 if data:
-                    sys.stderr.write(data)
-                    sys.stderr.flush()
+                    err.write(data)
+                    err.flush()
             if session.exit_status_ready():
                 break
         session.close()
@@ -598,7 +604,6 @@
         Use this to run commands that require local SSH credentials. For
         example, getting private branches from Launchpad.
         """
-        cmd = cmd % self._instance._vals
         self._instance.log(
             '%s@%s$ %s\n'
             % (self._username, self._instance._boto_instance.id, cmd))

=== modified file 'lib/devscripts/ec2test/testrunner.py'
--- lib/devscripts/ec2test/testrunner.py	2009-09-27 20:32:54 +0000
+++ lib/devscripts/ec2test/testrunner.py	2009-10-04 16:05:20 +0000
@@ -33,26 +33,6 @@
             self,
             "Couldn't parse '%s', not a Launchpad branch." % (branch_url,))
 
-def validate_file(filename):
-    """Raise an error if 'filename' is not a file we can write to."""
-    if filename is None:
-        return
-
-    check_file = filename
-    if os.path.exists(check_file):
-        if not os.path.isfile(check_file):
-            raise ValueError(
-                'file argument %s exists and is not a file' % (filename,))
-    else:
-        check_file = os.path.dirname(check_file)
-        if (not os.path.exists(check_file) or
-            not os.path.isdir(check_file)):
-            raise ValueError(
-                'file %s cannot be created.' % (filename,))
-    if not os.access(check_file, os.W_OK):
-        raise ValueError(
-            'you do not have permission to write %s' % (filename,))
-
 
 def parse_branch_url(branch_url):
     """Given the URL of a branch, return its components in a dict."""
@@ -131,11 +111,11 @@
 
     def __init__(self, branch, email=False, file=None, test_options='-vv',
                  headless=False, branches=(),
-                 machine_id=None,
                  pqm_message=None, pqm_public_location=None,
                  pqm_submit_location=None,
                  open_browser=False, pqm_email=None,
-                 include_download_cache_changes=None, instance=None, vals=None):
+                 include_download_cache_changes=None, instance=None,
+                 launchpad_login=None):
         """Create a new EC2TestRunner.
 
         This sets the following attributes:
@@ -148,29 +128,15 @@
           - message (after validating PQM submisson)
           - email (after validating email capabilities)
           - image (after connecting to ec2)
-          - file (after checking we can write to it)
-          - vals, a dict containing
-            - the environment
-            - trunk_branch (either from global or derived from branches)
-            - branch
-            - smtp_server
-            - smtp_username
-            - smtp_password
-            - email (distinct from the email attribute)
-            - key_type
-            - key
-            - launchpad_login
+          - file
         """
-        self.original_branch = branch # just for easy access in debugging
+        self.original_branch = branch
         self.test_options = test_options
         self.headless = headless
         self.include_download_cache_changes = include_download_cache_changes
         self.open_browser = open_browser
-
-        if test_options != '-vv' and pqm_message is not None:
-            raise ValueError(
-                "Submitting to PQM with non-default test options isn't "
-                "supported")
+        self.file = file
+        self._launchpad_login = launchpad_login
 
         trunk_specified = False
         trunk_branch = TRUNK_BRANCH
@@ -195,7 +161,7 @@
             if user == 'launchpad-pqm':
                 trunk_specified = True
             trunk_branch = src
-
+        self._trunk_branch = trunk_branch
         self.branches = branches.items()
 
         # XXX: JonathanLange 2009-05-31: The trunk_specified stuff above and
@@ -296,6 +262,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:
             if email is True:
                 email = [config.username()]
@@ -308,54 +276,32 @@
                 for item in email:
                     if not isinstance(item, basestring):
                         raise ValueError(
-                            'email must be True, False, a string, or a list of '
-                            'strings')
+                            'email must be True, False, a string, or a list '
+                            'of strings')
                     tmp.append(item)
                 email = tmp
         else:
             email = None
         self.email = email
 
-        # We do a lot of looking before leaping here because we want to avoid
-        # wasting time and money on errors we could have caught early.
-
-        # Validate and set file.
-        validate_file(file)
-        self.file = file
-
-        # Make a dict for string substitution based on the environ.
-        #
-        # XXX: JonathanLange 2009-06-02: Although this defintely makes the
-        # scripts & commands easier to write, it makes it harder to figure out
-        # how the different bits of the system interoperate (passing 'vals' to
-        # a method means it uses...?). Consider changing things around so that
-        # vals is not needed.
-        self.vals = vals
-        self.vals['trunk_branch'] = trunk_branch
-        self.vals['branch'] = branch
-
         # Email configuration.
         if email is not None or pqm_message is not None:
-            server = self.vals['smtp_server'] = config.get_user_option(
-                'smtp_server')
-            if server is None or server == 'localhost':
+            self._smtp_server = config.get_user_option('smtp_server')
+            if self._smtp_server is None or self._smtp_server == 'localhost':
                 raise ValueError(
                     'To send email, a remotely accessible smtp_server (and '
                     'smtp_username and smtp_password, if necessary) must be '
                     'configured in bzr.  See the SMTP server information '
                     'here: https://wiki.canonical.com/EmailSetup .')
-            self.vals['smtp_username'] = config.get_user_option(
-                'smtp_username')
-            self.vals['smtp_password'] = config.get_user_option(
-                'smtp_password')
+            self._smtp_username = config.get_user_option('smtp_username')
+            self._smtp_password = config.get_user_option('smtp_password')
             from_email = config.username()
             if not from_email:
+                # XXX: JonathanLange 2009-10-04: Is this strictly true? I
+                # can't actually see where this is used.
                 raise ValueError(
                     'To send email, your bzr email address must be set '
                     '(use ``bzr whoami``).')
-            else:
-                self.vals['email'] = (
-                    from_email.encode('utf8').encode('string-escape'))
 
         self._instance = instance
 
@@ -366,7 +312,6 @@
         sys.stdout.write(msg)
         sys.stdout.flush()
 
-
     def configure_system(self):
         user_connection = self._instance.connect()
         as_user = user_connection.perform
@@ -376,13 +321,13 @@
             bazaar_conf_file = user_connection.sftp.open(
                 ".bazaar/bazaar.conf", 'w')
             bazaar_conf_file.write(
-                'smtp_server = %(smtp_server)s\n' % self.vals)
-            if self.vals['smtp_username']:
-                bazaar_conf_file.write(
-                    'smtp_username = %(smtp_username)s\n' % self.vals)
-            if self.vals['smtp_password']:
-                bazaar_conf_file.write(
-                    'smtp_password = %(smtp_password)s\n' % self.vals)
+                'smtp_server = %s\n' % (self._smtp_server,))
+            if self._smtp_username:
+                bazaar_conf_file.write(
+                    'smtp_username = %s\n' % (self._smtp_username,))
+            if self._smtp_password:
+                bazaar_conf_file.write(
+                    'smtp_password = %s\n' % (self._smtp_password,))
             bazaar_conf_file.close()
         # Copy remote ec2-remote over
         self.log('Copying ec2test-remote.py to remote machine.\n')
@@ -391,7 +336,7 @@
                          'ec2test-remote.py'),
             '/var/launchpad/ec2test-remote.py')
         # Set up launchpad login and email
-        as_user('bzr launchpad-login %(launchpad-login)s')
+        as_user('bzr launchpad-login %s' % (self._launchpad_login,))
         user_connection.close()
 
     def prepare_tests(self):
@@ -400,11 +345,11 @@
         user_connection.perform('rm -rf /var/launchpad/test')
         # Get trunk.
         user_connection.run_with_ssh_agent(
-            'bzr branch %(trunk_branch)s /var/launchpad/test')
+            'bzr branch %s /var/launchpad/test' % (self._trunk_branch,))
         # Merge the branch in.
-        if self.vals['branch'] is not None:
+        if self._branch is not None:
             user_connection.run_with_ssh_agent(
-                'cd /var/launchpad/test; bzr merge %(branch)s')
+                'cd /var/launchpad/test; bzr merge %s' % (self._branch,))
         else:
             self.log('(Testing trunk, so no branch merge.)')
         # get newest sources
@@ -530,14 +475,14 @@
             cmd.append('--daemon')
 
         # Which branch do we want to test?
-        if self.vals['branch'] is not None:
-            branch = self.vals['branch']
+        if self._branch is not None:
+            branch = self._branch
             remote_branch = Branch.open(branch)
             branch_revno = remote_branch.revno()
         else:
-            branch = self.vals['trunk_branch']
+            branch = self._trunk_branch
             branch_revno = None
-        cmd.append('--public-branch=%s'  % branch)
+        cmd.append('--public-branch=%s' % branch)
         if branch_revno is not None:
             cmd.append('--public-branch-revno=%d' % branch_revno)