Code review comment for lp:~milo/lava-dispatcher/tar-repo

Hi Milo,

This looks nice. I have a few comments to be addressed.

 review needs-fixing

> === modified file 'lava_dispatcher/actions/lava_test_shell.py'
> --- lava_dispatcher/actions/lava_test_shell.py 2013-07-17 09:16:46 +0000
> +++ lava_dispatcher/actions/lava_test_shell.py 2013-07-22 13:39:27 +0000
> @@ -107,13 +107,16 @@
>
> from datetime import datetime
> from glob import glob
> +import base64
> import logging
> import os
> import pexpect
> import pkg_resources
> import shutil
> import stat
> +import StringIO
> import subprocess
> +import tarfile
> import tempfile
> import time
> from uuid import uuid4
> @@ -187,6 +190,37 @@
> logging.error('Unable to get test definition from bzr\n' + str(e))
>
>
> +def _get_testdef_tar_repo(testdef_repo, tmpdir):
> + """Extracts the provided encoded tar archive into tmpdir."""
> + tardir = os.path.join(tmpdir, 'tartestrepo')
> + temp_tar = os.path.join(tmpdir, "tar-repo.tar")
> +
> + try:
> + if not os.path.isdir(tardir):
> + logging.info("Creating directory to extract the tar archive into.")
> + os.makedirs(tardir)
> +
> + encoded_in = StringIO.StringIO(testdef_repo)
> + decoded_out = StringIO.StringIO()
> + base64.decode(encoded_in, decoded_out)
> +
> + # The following two operations can also be done in memory
> + # using cStringIO.
> + # At the moment the tar file sent is not big, but that can change.
> + with open(temp_tar, "w") as write_tar:
> + write_tar.write(decoded_out.getvalue())
> +
> + with tarfile.open(temp_tar) as tar:
> + tar.extractall(path=tardir)
> + except Exception as ex:

can we capture a more specific type of exception here? otherwise we may be
masking random exceptions that might happen above.

> + logging.error("Error extracting the tar archive.\n" + str(ex))
> + finally:
> + # Remove the temporary created tar file after it has been extracted.
> + if os.path.isfile(temp_tar):
> + os.unlink(temp_tar)
> + return tardir
> +
> +
> def _get_testdef_info(testdef):
> metadata = {'os': '', 'devices': '', 'environment': ''}
> metadata['description'] = testdef['metadata'].get('description')
> @@ -255,8 +289,20 @@
> name = testdef_repo['bzr-repo'].replace('lp:', '').split('/')[-1]
> info = _bzr_info(testdef_repo['bzr-repo'], repo, name)
>
> + if 'tar-repo' in testdef_repo:
> + repo = _get_testdef_tar_repo(testdef_repo['tar-repo'], tmpdir)
> + # Default info structure, since we need something, but we have
> + # a tar file in this case.
> + info = {
> + "project_name": "Tar archived repository",
> + "branch_vcs": "tar",
> + "branch_revision": "0",
> + "branch_url": repo
> + }
> +
> if not repo or not info:
> logging.debug("Unable to identify specified repository. %s" % testdef_repo)
> +
> test = testdef_repo.get('testdef', 'lavatest.yaml')
> with open(os.path.join(repo, test), 'r') as f:
> logging.info('loading test definition ...')
> @@ -462,13 +508,15 @@
> 'items': {'type': 'object',
> 'properties':
> {'git-repo': {'type': 'string',
> - 'optional': True},
> - 'bzr-repo': {'type': 'string',
> - 'optional': True},
> - 'revision': {'type': 'string',
> - 'optional': True},
> - 'testdef': {'type': 'string',
> - 'optional': True}
> + 'optional': True},
> + 'bzr-repo': {'type': 'string',
> + 'optional': True},
> + 'tar-repo': {'type': 'string',
> + 'optional': True},
> + 'revision': {'type': 'string',
> + 'optional': True},
> + 'testdef': {'type': 'string',
> + 'optional': True}

I think this indentation here got changed with a recent PEP8-related merge so
that this will not merge cleanly, please check.