Hi Milo,

Very nice progress here! I have some comments below to help improving it.

 review needs-fixing

Also I wonder whether we should merge my branch into trunk already so
you don't keep developing off the official places ... it's not like
there will be a release before this work is done with.

> === modified file '.bzrignore'
> --- .bzrignore	2013-06-10 15:38:27 +0000
> +++ .bzrignore	2013-06-10 15:38:27 +0000
> @@ -3,3 +3,6 @@
>  ./build
>  ./dist
>  /tags
> +.testrepository
> +*.egg
> +ci-build-venv

you won't need the last one anymore.

> === added file '.testr.conf'
> --- .testr.conf	1970-01-01 00:00:00 +0000
> +++ .testr.conf	2013-06-10 15:38:27 +0000
> @@ -0,0 +1,3 @@
> +[DEFAULT]
> +test_command=python -m subunit.run $IDLIST
> +test_id_list_default=lava_tool.tests.test_suite
> 
> === modified file 'ci-build'
> --- ci-build	2013-06-10 15:38:27 +0000
> +++ ci-build	2013-06-10 15:38:27 +0000
> @@ -1,11 +1,13 @@
>  #!/bin/sh
>  
> +VENV_DIR="/tmp/ci-build-venv"
> +
>  set -e
>  
>  if test -z "$VIRTUAL_ENV"; then
>    set -x
> -  virtualenv ci-build-venv
> -  . ci-build-venv/bin/activate
> +  virtualenv $VENV_DIR
> +  . $VENV_DIR/bin/activate
>    python setup.py develop
>  fi
>  
> @@ -21,6 +23,22 @@
>    pip install mocker
>  fi
>  
> +if ! pip show pep8 | grep -q pep8; then
> +  pip install pep8
> +fi
> +
> +if ! pip show pyflakes | grep -q pyflakes; then
> +  pip install pyflakes
> +fi
> +
> +if ! pip show testtools | grep -q testtools; then
> +  pip install testtools
> +fi
> +
> +if ! pip show mock | grep -q mock; then
> +  pip install mock
> +fi
> +
>  export LAVACONFIG=/dev/null
>  
>  if test -z "$DISPLAY"; then
> 

if you dropped pyflakes/pep8 tests there is no point in installing them
here.

> === added directory 'lava/device'
> === added file 'lava/device/__init__.py'
> --- lava/device/__init__.py	1970-01-01 00:00:00 +0000
> +++ lava/device/__init__.py	2013-06-10 15:38:27 +0000
> @@ -0,0 +1,112 @@
> +# Copyright (C) 2013 Linaro Limited
> +#
> +# Author: Milo Casagrande <email address hidden>
> +#
> +# This file is part of lava-tool.
> +#
> +# lava-tool is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU Lesser General Public License version 3
> +# as published by the Free Software Foundation
> +#
> +# lava-tool is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU Lesser General Public License
> +# along with lava-tool.  If not, see <http://www.gnu.org/licenses/>.
> +
> +import re
> +
> +from lava.device.templates import DEFAULT_TEMPLATE
> +from lava.tool.errors import CommandError
> +
> +
> +class Device(object):
> +    """A generic device."""
> +    def __init__(self, name):
> +        super(Device, self).__init__()

I don't think object has anything useful in its constructor.

> +        self.device_type = None
> +        self.hostname = name
> +        self.template = DEFAULT_TEMPLATE

In _update you are changing this object, you might want to make a copy instead.

> +
> +    def write(self, where):
> +        """Writes the object to file.
> +
> +        :param where: The full path of the file where to write."""
> +        with open(where, 'w') as f:
> +            f.write(self.__str__())

renaming `where` to `file` will make the code a lot more obvious.

> +
> +    def _update(self):
> +        """Updates the template with the values specified for this class.
> +
> +        Subclasses need to override this when they add more specific
> +        attributes.
> +        """
> +        self.template.update(device_type=self.device_type,
> +                             hostname=self.hostname)

This will change lava.device.template.DEFAULT_TEMPLATE since self.template is a
reference to it. You might want to create a copy instead.

> +
> +    def __str__(self):
> +        self._update()
> +        string = ""
> +        for key, value in self.template.iteritems():
> +            if not value:
> +                value = ''
> +            string += "%s = %s\n" % (str(key), str(value))
> +        return string
> +
> +    def __repr__(self):
> +        self._update()
> +        return str(self.template)
> +
> +
> +class PandaDevice(Device):
> +    """A panda device."""
> +    def __init__(self, name):
> +        super(PandaDevice, self).__init__(name)
> +        self.device_type = 'panda'

I'm not sure we gain anything with this class. just creating instances of
Device with the appropriate data should be enough

> +# Dictionary with key the name of a know device, and value a tuple composed of
> +# a matcher used to guess the device type, and its associated Device class.
> +known_devices = {
> +    'panda': (re.compile('^.*panda.*', re.I), PandaDevice),
> +}

you might create this dynamically from a list of device type names.  I don't
see us needing one class per device type, so perhaps the last element in the
value tuple should be a template, which could be looked up by device type name
in lava.device.templates, or a default if the device type name does not bring
anything.

> +
> +
> +def _get_device_type_from_user():
> +    """Makes the user write what kind of device this is.
> +
> +    If something goes wrong, raises CommandError.
> +    """
> +    try:
> +        dev_type = raw_input("Please specify the device type: ").strip()
> +    except (EOFError, KeyboardInterrupt):
> +        dev_type = None
> +    if not dev_type:
> +        raise CommandError("DEVICE name not specified or not correct.")
> +    return dev_type
> +
> +
> +def get_known_device(device):
> +    """Tries to match a device name with a known device type.
> +
> +    :param device: The name of the device we want matched to a real device.
> +    :return Its special Device instance, or a general one.
> +        """
> +    instance = None
> +    for known_dev, (matcher, clazz) in known_devices.iteritems():
> +        if matcher.match(device):
> +            instance = clazz(device)
> +    if not instance:
> +        dev_type = _get_device_type_from_user()
> +        known_dev = known_devices.get(dev_type, None)
> +        if known_dev:
> +            clazz = known_dev[1]
> +            instance = clazz(device)
> +        else:
> +            print ("Device '%s' does not match a known device." % dev_type)
> +            instance = Device(device)
> +            instance.device_type = dev_type
> +
> +    return instance
> 
> === added file 'lava/device/commands.py'
> --- lava/device/commands.py	1970-01-01 00:00:00 +0000
> +++ lava/device/commands.py	2013-06-10 15:38:27 +0000
> @@ -0,0 +1,177 @@
> +# Copyright (C) 2013 Linaro Limited
> +#
> +# Author: Milo Casagrande <email address hidden>
> +#
> +# This file is part of lava-tool.
> +#
> +# lava-tool is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU Lesser General Public License version 3
> +# as published by the Free Software Foundation
> +#
> +# lava-tool is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU Lesser General Public License
> +# along with lava-tool.  If not, see <http://www.gnu.org/licenses/>.
> +
> +import os
> +import subprocess
> +import sys
> +
> +from lava.config import InteractiveConfig, Parameter
> +from lava.device import get_known_device
> +from lava.tool.command import Command, CommandGroup
> +from lava.tool.errors import CommandError
> +from lava_tool.utils import has_command
> +
> +
> +# Default lava-dispatcher path, has to be joined with an instance full path.
> +DEFAUL_DISPATCHER_PATH = os.path.join('etc', 'lava-dispatcher')

missing a T in DEFAULT

> +# Default devices path.
> +DEFAULT_DEVICES_PATH = 'devices'
> +DEVICE_FILE_SUFFIX = "conf"
> +
> +
> +class device(CommandGroup):
> +    """LAVA devices handling"""
> +
> +    namespace = "lava.device.commands"
> +
> +
> +class BaseCommand(Command):
> +    def __init__(self, parser, args):
> +        super(BaseCommand, self).__init__(parser, args)
> +        self.config = InteractiveConfig(force_interactive=self.args.interactive)
> +
> +    @classmethod
> +    def register_arguments(cls, parser):
> +        super(BaseCommand, cls).register_arguments(parser)
> +        parser.add_argument("-i", "--interactive",
> +                            action='store_true',
> +                            help=("Forces asking for input parameters even if "
> +                                  "we already have them cached."))
> +
> +    def _get_devices_path(self):
> +        """Gets the path to the devices in the LAVA dispatcher."""
> +        lava_instance = self.config.get(Parameter("lava_instance"))
> +        if not lava_instance:
> +            # Nothing provided? We write on the current dir.
> +            lava_instance = os.getcwd()
> +            print "LAVA instance path will be: %s" % lava_instance
> +
> +        dispatcher_path = os.path.join(lava_instance, DEFAUL_DISPATCHER_PATH)
> +        devices_path = os.path.join(dispatcher_path, DEFAULT_DEVICES_PATH)
> +
> +        self._create_devices_path(devices_path)
> +
> +        return devices_path

I think you cannot always assume an instance here. If you have lava-tool
installed with Debian packages, then you don't have a local instance. The
problem is that the dispatcher does not expose a list of configuration
directories that we can reference. I will work on this tomorrow, and will let
you know.

> === added file 'lava/device/templates.py'
> --- lava/device/templates.py	1970-01-01 00:00:00 +0000
> +++ lava/device/templates.py	2013-06-10 15:38:27 +0000
> @@ -0,0 +1,28 @@
> +# Copyright (C) 2013 Linaro Limited
> +#
> +# Author: Milo Casagrande <email address hidden>
> +#
> +# This file is part of lava-tool.
> +#
> +# lava-tool is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU Lesser General Public License version 3
> +# as published by the Free Software Foundation
> +#
> +# lava-tool is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU Lesser General Public License
> +# along with lava-tool.  If not, see <http://www.gnu.org/licenses/>.
> +
> +"""
> +This is just a place where to store a template like dictionary that
> +will be used to serialize a Device object.
> +"""
> +
> +DEFAULT_TEMPLATE = {
> +    'device_type': None,
> +    'hostname': None,
> +    'connection_command': None
> +}

I think we could keep a dictionary of templates, indexed by device type (see
also my comments above wrt lava.device.__init__.py)

> === added file 'lava_tool/utils.py'
> --- lava_tool/utils.py	1970-01-01 00:00:00 +0000
> +++ lava_tool/utils.py	2013-06-10 15:38:27 +0000
> @@ -0,0 +1,37 @@
> +# Copyright (C) 2013 Linaro Limited
> +#
> +# Author: Milo Casagrande <email address hidden>
> +#
> +# This file is part of lava-tool.
> +#
> +# lava-tool is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU Lesser General Public License version 3
> +# as published by the Free Software Foundation
> +#
> +# lava-tool is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU Lesser General Public License
> +# along with lava-tool.  If not, see <http://www.gnu.org/licenses/>.
> +
> +import subprocess
> +
> +
> +def has_command(command):
> +    """Checks that the given command is available.
> +
> +    :param command: The name of the command to check availability.
> +    """
> +    command_available = True
> +    try:
> +        cmd_args = ["which", command]
> +        process = subprocess.Popen(cmd_args, stdout=open('/dev/null', 'w'))
> +        process.wait()
> +        if process.returncode != 0:
> +            command_available = False
> +    except subprocess.CalledProcessError:
> +        # TODO: logging infrastructure?
> +        command_available = False
> +    return command_available

CalledProcessError will never be raised here. It's only raised by check_call
and check_output.

try this:

def has_command(cmd):
    try:
        subprocess.check_call(['which', cmd], stdout=open('/dev/null', 'w'))
        return True
    except subprocess.CalledProcessError:
        return False

there should be a function in the stdlib for this ...

> === modified file 'setup.py'
> --- setup.py	2013-06-10 15:38:27 +0000
> +++ setup.py	2013-06-10 15:38:27 +0000
> @@ -53,5 +53,11 @@
>          'versiontools >= 1.3.1'
>      ],
>      setup_requires=['versiontools >= 1.3.1'],
> -    tests_require=['mocker >= 1.0'],
> +    tests_require=[
> +        'mocker >= 1.0',
> +        'pep8',
> +        'pyflakes',
> +        'testtools == 0.9.15',
> +        'mock >= 0.7.2'
> +    ],
>      zip_safe=True)

so do we depend or not on pyflakes/pep8?