Merge lp:~benji/lpsetup/add-get-subcommand into lp:lpsetup

[Benji York]

create a "get" command by cannibalizing the "update" and "install" commands

This branch moves all the steps out of "update" and one out of "install" into a new "get" command and then refactors "update" and "install" to continue to work as before.

Tests: the existing tests pass, no new tests were added; the install and update commands were run to check that they still appear to work as well as running the new "get" command.

Lint: the lint report is clean

[Brad Crittenden (bac)]

* Please add a period to the file docstring.

* The LP_REPOS[1] vs [0] part in setup_codebase is confusing. Perhaps if you assigned it to a descriptive variable and used it in the command it might be easier to understand/self-documenting.

* Do we need to specify --2a for init-repo?

I know those existed in the code you refactored.

Otherwise it looks great.

review: Approve

[Benji York (benji)]

> * Please add a period to the file docstring.

Done.

> * The LP_REPOS[1] vs [0] part in setup_codebase is confusing. Perhaps if you
> assigned it to a descriptive variable and used it in the command it might be
> easier to understand/self-documenting.

Done.

> * Do we need to specify --2a for init-repo?

I have no idea. As sou note, that was pre-existing. I have emailed
frankban to see why it is there and have asked that he add a comment
about the motivation if it is.

[Brad Crittenden (bac)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

[Gary Poster (gary)]

I added a kanban card for adding he lightweight checkout support.

There are a number of other important details in the bug/kanban description that have not yet been implemented, as far as I can tell. The repository argument, which should mean that it is created only if needed, looks missing to me–it is always created. The destination directory is hard coded. In general, the command needs to be reviewed to fit into the requested plan. If the plan is unclear, that's very understandable, but we still need to try and share knowledge and follow it.

This is probably from the original, but i'd also like to look through the whole file to make sure bar terminology is used correctly, be i branch, repository, or checkout.

=== modified file 'lpsetup/cli.py'
--- lpsetup/cli.py	2012-06-21 14:37:02 +0000
+++ lpsetup/cli.py	2012-06-25 19:27:19 +0000
@@ -16,6 +16,7 @@
     )
 from lpsetup.subcommands import (
     branch,
+    get,
     inithost,
     install,
     lxcinstall,
@@ -28,6 +29,7 @@
     parser = argparser.ArgumentParser(description=description)
     parser.register_subcommand('install', install.SubCommand)
     parser.register_subcommand('inithost', inithost.SubCommand)
+    parser.register_subcommand('get', get.SubCommand)
     parser.register_subcommand('update', update.SubCommand)
     parser.register_subcommand('lxc-install', lxcinstall.SubCommand)
     parser.register_subcommand('branch', branch.SubCommand)

=== added file 'lpsetup/subcommands/get.py'
--- lpsetup/subcommands/get.py	1970-01-01 00:00:00 +0000
+++ lpsetup/subcommands/get.py	2012-06-25 19:27:19 +0000
@@ -0,0 +1,241 @@
+#!/usr/bin/env python
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""get subcommand: prepare source code destinations and download it."""
+
+__metaclass__ = type
+__all__ = [
+    'setup_codebase',
+    'SubCommand',
+    ]
+
+import os
+import subprocess
+
+from shelltoolbox import (
+    cd,
+    get_su_command,
+    mkdirs,
+    run,
+    su,
+    )
+
+from lpsetup import (
+    argparser,
+    handlers,
+    )
+from lpsetup.settings import (
+    CHECKOUT_DIR,
+    DEPENDENCIES_DIR,
+    LP_CHECKOUT,
+    LP_REPOS,
+    LP_SOURCE_DEPS,
+    SSH_KEY_NAME,
+    )
+from lpsetup.utils import call
+
+
+def setup_codebase(user, checkout_dir, dependencies_dir, valid_ssh_keys=True):
+    """Set up Launchpad repository and source dependencies.
+
+    Return True if new changes are pulled from bzr repository.
+    """
+    # TODO If doing no trees, use --lightweight.
+    # Using real su because bzr uses uid.
+    if os.path.exists(checkout_dir):
+        # Pull the repository.
+        revno_args = ('bzr', 'revno', checkout_dir)
+        revno = run(*revno_args)
+        call(*get_su_command(user, ['bzr', 'pull', '-d', checkout_dir]))
+        changed = revno != run(*revno_args)
+    else:
+        # Branch the repository.
+        # If we have valid SSH keys we should use the "lp:" style URL, "http"
+        # if not.
+        if valid_ssh_keys:
+            repo = LP_REPOS[1]
+        else:
+            repo = LP_REPOS[0]
+        cmd = ('bzr', 'branch', repo, checkout_dir)
+        call(*get_su_command(user, cmd))
+        changed = True
+    # Check repository integrity.
+    if subprocess.call(['bzr', 'status', '-q', checkout_dir]):
+        raise subprocess.CalledProcessError(
+            'Repository {0} is corrupted.'.format(checkout_dir))
+    # Set up source dependencies.
+    with su(user):
+        for subdir in ('eggs', 'yui', 'sourcecode'):
+            mkdirs(os.path.join(dependencies_dir, subdir))
+        download_cache = os.path.join(dependencies_dir, 'download-cache')
+        if os.path.exists(download_cache):
+            call('bzr', 'up', download_cache)
+        else:
+            call('bzr', 'co', '--lightweight', LP_SOURCE_DEPS, download_cache)
+    return changed
+
+
+def fetch(user, directory, dependencies_dir, valid_ssh_keys):
+    """Create a repo for the Launchpad code and retrieve it."""
+    # TODO Add --no-trees handling.
+    with su(user):
+        # Set up the repository.
+        mkdirs(directory)
+        call('bzr', 'init-repo', '--2a', directory)
+    # Set up the codebase.
+    checkout_dir = os.path.join(directory, LP_CHECKOUT)
+    setup_codebase(user, checkout_dir, dependencies_dir, valid_ssh_keys)
+
+
+def setup_external_sourcecode(
+    user, checkout_dir, dependencies_dir, valid_ssh_keys=True):
+    """Update and link external sourcecode."""
+    cmd = (
+        'utilities/update-sourcecode',
+        None if valid_ssh_keys else '--use-http',
+        os.path.join(dependencies_dir, 'sourcecode'),
+        )
+    with cd(checkout_dir):
+        # Using real su because update-sourcecode uses uid.
+        call(*get_su_command(user, cmd))
+        with su(user):
+            call('utilities/link-external-sourcecode', dependencies_dir)
+
+
+def make_launchpad(user, checkout_dir, install=False):
+    """Make and optionally install Launchpad."""
+    # Using real su because mailman make script uses uid.
+    call(*get_su_command(user, ['make', '-C', checkout_dir]))
+    if install:
+        call('make', '-C', checkout_dir, 'install')
+
+
+def update_launchpad(user, dependencies_dir, directory, make_schema, apt):
+    """Update the Launchpad environment."""
+    if apt:
+        call('apt-get', 'update')
+        call('apt-get', 'upgrade')
+    checkout_dir = os.path.join(directory, LP_CHECKOUT)
+    # Update the Launchpad codebase.
+    changed = setup_codebase(user, checkout_dir, dependencies_dir)
+    setup_external_sourcecode(user, checkout_dir, dependencies_dir)
+    if changed:
+        make_launchpad(user, checkout_dir, install=False)
+        if make_schema:
+            with su(user):
+                call('make', '-C', checkout_dir, 'schema')
+
+
+def link_sourcecode_in_branches(user, dependencies_dir, directory):
+    """Link external sourcecode for all branches in the project."""
+    checkout_dir = os.path.join(directory, LP_CHECKOUT)
+    cmd = os.path.join(checkout_dir, 'utilities', 'link-external-sourcecode')
+    with su(user):
+        for dirname in os.listdir(directory):
+            branch = os.path.join(directory, dirname)
+            sourcecode_dir = os.path.join(branch, 'sourcecode')
+            if (branch != checkout_dir and
+                os.path.exists(sourcecode_dir) and
+                os.path.isdir(sourcecode_dir)):
+                call(cmd, '--parent', dependencies_dir, '--target', branch)
+
+
+class SubCommand(argparser.StepsBasedSubCommand):
+    """Prepare a host machine to run Launchpad."""
+
+    fetch_step = (fetch,
+         'user', 'directory', 'dependencies_dir', 'valid_ssh_keys')
+    update_launchpad_step = (update_launchpad,
+         'user', 'dependencies_dir', 'directory', 'make_schema', 'apt')
+    link_sourcecode_in_branches_step = (link_sourcecode_in_branches,
+         'user', 'dependencies_dir', 'directory')
+
+    steps = (
+        fetch_step,
+        update_launchpad_step,
+        link_sourcecode_in_branches_step,
+        )
+
+    help = __doc__
+    needs_root = True
+    validators = (
+        handlers.handle_user,
+        handlers.handle_lpuser,
+        handlers.handle_userdata,
+        handlers.handle_ssh_keys,
+        handlers.handle_directories,
+        )
+
+    def get_needs_root(self, namespace):
+        # Root is needed only if an apt update/upgrade is requested.
+        return namespace.apt
+
+    def add_update_arguments(self, parser):
+        parser.add_argument(
+            '-u', '--user',
+            help='The name of the system user that will own the branch. '
+                 'The current user is used if this script is not run as '
+                 'root and this argument is omitted.')
+        parser.add_argument(
+            '-c', '--directory', default=CHECKOUT_DIR,
+            help='The directory of the Launchpad repository to be updated. '
+                 'The directory must reside under the home directory of the '
+                 'given user (see -u argument). '
+                 '[DEFAULT={0}]'.format(CHECKOUT_DIR))
+        parser.add_argument(
+            '--make-schema', action='store_true',
+            help='Run `make schema` if code updates are found.')
+        parser.add_argument(
+            '-A', '--apt', action='store_true',
+            help='Also update deb packages.')
+        parser.add_argument(
+            '-d', '--dependencies-dir', default=DEPENDENCIES_DIR,
+            help='The directory of the Launchpad dependencies to be linked. '
+                 'The directory must reside under the home directory of the '
+                 'given user (see -u argument). '
+                 '[DEFAULT={0}]'.format(DEPENDENCIES_DIR))
+        parser.add_argument(
+            '-S', '--ssh-key-name', default=SSH_KEY_NAME,
+            help='{0} [DEFAULT={1}]'.format(
+                'The ssh key name used to connect to Launchpad.',
+                SSH_KEY_NAME))
+
+    def add_arguments(self, parser):
+        super(SubCommand, self).add_arguments(parser)
+        parser.add_argument(
+            '-e', '--email',
+            help='The email of the user, used for bzr whoami. This argument '
+                 'can be omitted if a bzr id exists for current user.')
+        parser.add_argument(
+            '-f', '--full-name',
+            help='The full name of the user, used for bzr whoami. '
+                 'This argument can be omitted if a bzr id exists for '
+                 'current user.')
+        parser.add_argument(
+            '-l', '--lpuser',
+            help='The name of the Launchpad user that will be used to '
+                 'check out dependencies. If not provided, the system '
+                 'user name is used.')
+        parser.add_argument(
+            '-v', '--private-key',
+            help='The SSH private key for the Launchpad user (without '
+                 'passphrase). If this argument is omitted and a keypair is '
+                 'not found in the home directory of the system user a new '
+                 'SSH keypair will be generated and the checkout of the '
+                 'Launchpad code will use HTTP rather than bzr+ssh.')
+        parser.add_argument(
+            '-b', '--public-key',
+            help='The SSH public key for the Launchpad user. '
+                 'If this argument is omitted and a keypair is not found '
+                 'in the home directory of the system user a new SSH '
+                 'keypair will be generated and the checkout of the '
+                 'Launchpad code will use HTTP rather than bzr+ssh.')
+        parser.add_argument(
+            '-N', '--no-repositories', action='store_true',
+            help='Do not add APT repositories.')
+        parser.add_argument(
+            '--feed-random', action='store_true',
+            help='Use haveged to maintain a pool of random bytes used to '
+                 'fill /dev/random and avoid entropy exhaustion.')
+        self.add_update_arguments(parser)

=== modified file 'lpsetup/subcommands/inithost.py'
--- lpsetup/subcommands/inithost.py	2012-06-22 15:01:49 +0000
+++ lpsetup/subcommands/inithost.py	2012-06-25 19:27:19 +0000
@@ -110,7 +110,6 @@
 
     help = __doc__
     needs_root = True
-    ssh_key_name_help = 'The ssh key name used to connect to Launchpad.'
     validators = (
         handlers.handle_user,
         handlers.handle_lpuser,
@@ -156,7 +155,8 @@
         parser.add_argument(
             '-S', '--ssh-key-name', default=SSH_KEY_NAME,
             help='{0} [DEFAULT={1}]'.format(
-                self.ssh_key_name_help, SSH_KEY_NAME))
+                'The ssh key name used to connect to Launchpad.',
+                SSH_KEY_NAME))
         parser.add_argument(
             '-N', '--no-repositories', action='store_true',
             help='Do not add APT repositories.')

=== modified file 'lpsetup/subcommands/install.py'
--- lpsetup/subcommands/install.py	2012-06-22 15:07:50 +0000
+++ lpsetup/subcommands/install.py	2012-06-25 19:27:19 +0000
@@ -6,11 +6,7 @@
 
 __metaclass__ = type
 __all__ = [
-    'fetch',
-    'make_launchpad',
     'setup_bzr_locations',
-    'setup_codebase',
-    'setup_external_sourcecode',
     'setup_launchpad',
     'SubCommand',
     ]
@@ -21,15 +17,16 @@
 
 from shelltoolbox import (
     cd,
-    get_su_command,
     file_append,
     mkdirs,
-    run,
     su,
     )
 
 from lpsetup import handlers
-from lpsetup.subcommands import inithost
+from lpsetup.subcommands import (
+    inithost,
+    get,
+    )
 from lpsetup.settings import (
     CHECKOUT_DIR,
     DEPENDENCIES_DIR,
@@ -39,8 +36,6 @@
     LP_APACHE_ROOTS,
     LP_BZR_LOCATIONS,
     LP_CHECKOUT,
-    LP_REPOS,
-    LP_SOURCE_DEPS,
     )
 from lpsetup.utils import (
     call,
@@ -48,74 +43,6 @@
     )
 
 
-def setup_codebase(user, checkout_dir, dependencies_dir, valid_ssh_keys=True):
-    """Set up Launchpad repository and source dependencies.
-
-    Return True if new changes are pulled from bzr repository.
-    """
-    # Using real su because bzr uses uid.
-    if os.path.exists(checkout_dir):
-        # Pull the repository.
-        revno_args = ('bzr', 'revno', checkout_dir)
-        revno = run(*revno_args)
-        call(*get_su_command(user, ['bzr', 'pull', '-d', checkout_dir]))
-        changed = revno != run(*revno_args)
-    else:
-        # Branch the repository.
-        cmd = ('bzr', 'branch',
-               LP_REPOS[1] if valid_ssh_keys else LP_REPOS[0], checkout_dir)
-        call(*get_su_command(user, cmd))
-        changed = True
-    # Check repository integrity.
-    if subprocess.call(['bzr', 'status', '-q', checkout_dir]):
-        raise subprocess.CalledProcessError(
-            'Repository {0} is corrupted.'.format(checkout_dir))
-    # Set up source dependencies.
-    with su(user):
-        for subdir in ('eggs', 'yui', 'sourcecode'):
-            mkdirs(os.path.join(dependencies_dir, subdir))
-        download_cache = os.path.join(dependencies_dir, 'download-cache')
-        if os.path.exists(download_cache):
-            call('bzr', 'up', download_cache)
-        else:
-            call('bzr', 'co', '--lightweight', LP_SOURCE_DEPS, download_cache)
-    return changed
-
-
-def setup_external_sourcecode(
-    user, checkout_dir, dependencies_dir, valid_ssh_keys=True):
-    """Update and link external sourcecode."""
-    cmd = (
-        'utilities/update-sourcecode',
-        None if valid_ssh_keys else '--use-http',
-        os.path.join(dependencies_dir, 'sourcecode'),
-        )
-    with cd(checkout_dir):
-        # Using real su because update-sourcecode uses uid.
-        call(*get_su_command(user, cmd))
-        with su(user):
-            call('utilities/link-external-sourcecode', dependencies_dir)
-
-
-def make_launchpad(user, checkout_dir, install=False):
-    """Make and optionally install Launchpad."""
-    # Using real su because mailman make script uses uid.
-    call(*get_su_command(user, ['make', '-C', checkout_dir]))
-    if install:
-        call('make', '-C', checkout_dir, 'install')
-
-
-def fetch(user, directory, dependencies_dir, valid_ssh_keys):
-    """Create a repo for the Launchpad code and retrieve it."""
-    with su(user):
-        # Set up the repository.
-        mkdirs(directory)
-        call('bzr', 'init-repo', '--2a', directory)
-    # Set up the codebase.
-    checkout_dir = os.path.join(directory, LP_CHECKOUT)
-    setup_codebase(user, checkout_dir, dependencies_dir, valid_ssh_keys)
-
-
 def setup_launchpad(user, dependencies_dir, directory, valid_ssh_keys):
     """Set up the Launchpad environment."""
     # User configuration.
@@ -124,7 +51,7 @@
     subprocess.call(['addgroup', '--gid', str(pwd_database.pw_gid), user])
     # Set up Launchpad dependencies.
     checkout_dir = os.path.join(directory, LP_CHECKOUT)
-    setup_external_sourcecode(
+    get.setup_external_sourcecode(
         user, checkout_dir, dependencies_dir, valid_ssh_keys)
     with su(user):
         # Create Apache document roots, to avoid warnings.
@@ -136,7 +63,7 @@
         # Launchpad database setup.
         call('utilities/launchpad-database-setup', user)
     # Make and install launchpad.
-    make_launchpad(user, checkout_dir, install=True)
+    get.make_launchpad(user, checkout_dir, install=True)
     # Change owner of /srv/launchpad.dev/.
     os.chown('/srv/launchpad.dev/', pwd_database.pw_uid, pwd_database.pw_gid)
     # Set up container hosts file.
@@ -174,8 +101,7 @@
     # The steps for "install" are a superset of the steps for "inithost".
     steps = (
         inithost.SubCommand.initialize_step,
-        (fetch,
-         'user', 'directory', 'dependencies_dir', 'valid_ssh_keys'),
+        get.SubCommand.fetch_step,
         (setup_bzr_locations,
          'user', 'lpuser', 'directory'),
         inithost.SubCommand.setup_apt_step,

=== modified file 'lpsetup/subcommands/lxcinstall.py'
--- lpsetup/subcommands/lxcinstall.py	2012-06-22 15:07:50 +0000
+++ lpsetup/subcommands/lxcinstall.py	2012-06-25 19:27:19 +0000
@@ -228,8 +228,6 @@
          'lxc_name', 'ssh_key_path'),
         )
     help = __doc__
-    ssh_key_name_help = ('The ssh key name used to connect to Launchpad '
-                         'and to to the LXC container.')
 
     def get_validators(self, namespace):
         validators = super(SubCommand, self).get_validators(namespace)

=== modified file 'lpsetup/subcommands/update.py'
--- lpsetup/subcommands/update.py	2012-05-24 15:56:39 +0000
+++ lpsetup/subcommands/update.py	2012-06-25 19:27:19 +0000
@@ -6,99 +6,28 @@
 
 __metaclass__ = type
 __all__ = [
-    'link_sourcecode_in_branches',
-    'update_launchpad',
     'SubCommand',
     ]
 
-import os
-
-from shelltoolbox import su
-
-from lpsetup import (
-    argparser,
-    handlers,
-    )
-from lpsetup.settings import (
-    CHECKOUT_DIR,
-    DEPENDENCIES_DIR,
-    LP_CHECKOUT,
-    )
-from lpsetup.subcommands import install
-from lpsetup.utils import call
-
-
-def update_launchpad(user, dependencies_dir, directory, make_schema, apt):
-    """Update the Launchpad environment."""
-    if apt:
-        call('apt-get', 'update')
-        call('apt-get', 'upgrade')
-    checkout_dir = os.path.join(directory, LP_CHECKOUT)
-    # Update the Launchpad codebase.
-    changed = install.setup_codebase(user, checkout_dir, dependencies_dir)
-    install.setup_external_sourcecode(user, checkout_dir, dependencies_dir)
-    if changed:
-        install.make_launchpad(user, checkout_dir, install=False)
-        if make_schema:
-            with su(user):
-                call('make', '-C', checkout_dir, 'schema')
-
-
-def link_sourcecode_in_branches(user, dependencies_dir, directory):
-    """Link external sourcecode for all branches in the project."""
-    checkout_dir = os.path.join(directory, LP_CHECKOUT)
-    cmd = os.path.join(checkout_dir, 'utilities', 'link-external-sourcecode')
-    with su(user):
-        for dirname in os.listdir(directory):
-            branch = os.path.join(directory, dirname)
-            sourcecode_dir = os.path.join(branch, 'sourcecode')
-            if (branch != checkout_dir and
-                os.path.exists(sourcecode_dir) and
-                os.path.isdir(sourcecode_dir)):
-                call(cmd, '--parent', dependencies_dir, '--target', branch)
-
-
-class SubCommand(argparser.StepsBasedSubCommand):
+from lpsetup import handlers
+from lpsetup.subcommands import get
+
+
+class SubCommand(get.SubCommand):
     """Update the Launchpad environment to latest version."""
 
     steps = (
-        (update_launchpad,
-         'user', 'dependencies_dir', 'directory', 'make_schema', 'apt'),
-        (link_sourcecode_in_branches,
-         'user', 'dependencies_dir', 'directory'),
+        get.SubCommand.fetch_step,
+        get.SubCommand.update_launchpad_step,
+        get.SubCommand.link_sourcecode_in_branches_step,
         )
     help = __doc__
     validators = (
         handlers.handle_user,
         handlers.handle_directories,
+        handlers.handle_ssh_keys,
         )
 
-    def get_needs_root(self, namespace):
-        # Root is needed only if an apt update/upgrade is requested.
-        return namespace.apt
-
     def add_arguments(self, parser):
-        super(SubCommand, self).add_arguments(parser)
-        parser.add_argument(
-            '-u', '--user',
-            help='The name of the system user used to update Launchpad. '
-                 'The current user is used if this script is not run as '
-                 'root and this argument is omitted.')
-        parser.add_argument(
-            '-d', '--dependencies-dir', default=DEPENDENCIES_DIR,
-            help='The directory of the Launchpad dependencies to be updated. '
-                 'The directory must reside under the home directory of the '
-                 'given user (see -u argument). '
-                 '[DEFAULT={0}]'.format(DEPENDENCIES_DIR))
-        parser.add_argument(
-            '-c', '--directory', default=CHECKOUT_DIR,
-            help='The directory of the Launchpad repository to be updated. '
-                 'The directory must reside under the home directory of the '
-                 'given user (see -u argument). '
-                 '[DEFAULT={0}]'.format(CHECKOUT_DIR))
-        parser.add_argument(
-            '-S', '--make-schema', action='store_true',
-            help='Run `make schema` if code updates are found.')
-        parser.add_argument(
-            '-A', '--apt', action='store_true',
-            help='Also update deb packages.')
+        super(get.SubCommand, self).add_arguments(parser)
+        self.add_update_arguments(parser)