On Tue, Jul 16, 2013 at 07:41:29AM -0000, Milo Casagrande wrote:
> Milo Casagrande has proposed merging lp:~milo/lava-tool/lava-165 into lp:lava-tool with lp:~milo/lava-tool/device-parameters as a prerequisite.
> 
> Requested reviews:
>   Antonio Terceiro (terceiro)
> 
> For more details, see:
> https://code.launchpad.net/~milo/lava-tool/lava-165/+merge/174942
> 
> This adds final work towards completing the lava helper tools for card-15.
> The merge adds the following commands:
> 
> lava init [DIR]
> lava run [JOB|DIR]
> lava submit [JOB|DIR]
> lava update [JOB|DIR]
> 
> The "init" command initializes a directory with a job definition file, and a default subdirectory called "tests/" that will contain the test definition file(s), and a default shell script, called "mytest.sh". The test definition file steps, by default, will execute the shell script. The user is prompted to fill in the shell script.
> 
> The "run" command runs a job file on the local dispatcher, the "submit" one will send the job file to a remote LAVA server.
> 
> The "update" command can be used, in case the user modifies the content of the "tests/" directory, to update the job file.
> 
> The "run", "submit" and "update" commands can accept a job file (.json extension), a directory (it will pick up the first .json file) or nothing to use the current working directory.
> 
> New Parameter class have been introduced, not used at this stage, but where used with the first implementation. New attributes have also been added in order not to store the parameter on disk at program exit.
> 
> Tests have been added to cover most of the new features.

arf ... this one was big!

> === added file 'lava/commands.py'
> --- lava/commands.py	1970-01-01 00:00:00 +0000
> +++ lava/commands.py	2013-07-22 09:32:26 +0000
> @@ -0,0 +1,293 @@
> +# 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/>.
> +
> +"""
> +Lava init commands.
> +
> +When invoking:
> +
> + `lava init [DIR]`
> +
> +the command will create a default directory and files structure as follows:
> +
> +DIR/
> +   |
> +   +- JOB_FILE.json
> +   +- tests/
> +           |
> +           + mytest.sh
> +           + lavatest.yaml
> +
> +If DIR is not passed, it will use the current working directory.
> +JOB_FILE is a file name that will be asked to the user, along with
> +other necessary information to define the tests.
> +
> +If the user manually updates either the lavatest.yaml or mytest.sh file, it is
> +necessary to run the following command in order to update the job definition:
> +
> + `lava update [JOB|DIR]`
> +"""
> +
> +import copy
> +import json
> +import os
> +import sys
> +import stat
> +
> +from lava.helper.command import BaseCommand
> +from lava.helper.template import (
> +    expand_template,
> +    set_value
> +)
> +from lava.job.commands import JOB_FILE_EXTENSIONS
> +from lava.parameter import (
> +    Parameter,
> +    TarRepoParameter,
> +)
> +from lava.job.templates import (
> +    TESTDEF_REPOS_TAR_REPO,
> +)
> +from lava.testdef.templates import (
> +    DEFAULT_TESTDEF_FILE,
> +    DEFAULT_TESTDEF_SCRIPT,
> +    DEFAULT_TESTDEF_SCRIPT_CONTENT,
> +)
> +from lava.tool.errors import CommandError
> +
> +# Default directory structure name.
> +TESTS_DIR = "tests"
> +
> +# Internal parameter ids.
> +JOBFILE_ID = "jobfile"
> +
> +JOBFILE_PARAMETER = Parameter(JOBFILE_ID)
> +JOBFILE_PARAMETER.store = False
> +
> +INIT_TEMPLATE = {
> +    JOBFILE_ID: JOBFILE_PARAMETER,
> +}

Do we really need a dictionary with a single key?

> +
> +
> +class init(BaseCommand):
> +    """Set-ups the base directory structure."""
> +
> +    @classmethod
> +    def register_arguments(cls, parser):
> +        super(init, cls).register_arguments(parser)
> +        parser.add_argument("DIR",
> +                            help=("The name of the directory to initialize. "
> +                                  "Defaults to current working directory."),
> +                            nargs="?",
> +                            default=os.getcwd())
> +
> +    def invoke(self):
> +        full_path = os.path.abspath(self.args.DIR)
> +
> +        if os.path.isfile(full_path):
> +            raise CommandError("'{0}' already exists, and is a "
> +                               "file.".format(self.args.DIR))
> +
> +        if not os.path.isdir(full_path):
> +            try:
> +                os.makedirs(full_path)
> +            except OSError:
> +                raise CommandError("Cannot create directory "
> +                                   "'{0}'.".format(self.args.DIR))
> +
> +        test_path = os.path.join(full_path, TESTS_DIR)
> +        if not os.path.isdir(test_path):
> +            try:
> +                os.makedirs(test_path)
> +            except OSError:
> +                raise CommandError("Cannot create directory "
> +                                   "'{0}'.".format(self.args.DIR))
> +
> +        data = self._update_data()
> +        self._create_files(data, full_path, test_path)
> +
> +    def _update_data(self):
> +        """Updates the template and ask values to the user.
> +
> +        The template in this case is a layout of the directory structure as it
> +        would be written to disk.
> +
> +        :return A dictionary containing all the necessary file names to create.
> +        """
> +        data = copy.deepcopy(INIT_TEMPLATE)
> +        expand_template(data, self.config)
> +
> +        return data
> +
> +    def _create_files(self, data, full_path, test_path):
> +        # This is the default script file as defined in the testdef template.
> +        default_script = os.path.join(test_path, DEFAULT_TESTDEF_SCRIPT)
> +
> +        if not os.path.isfile(default_script):
> +            # We do not have the default testdef script. Create it, but
> +            # remind the user to update it.
> +            print >> sys.stdout, ("\nCreating default test script "
> +                                  "'{0}'.".format(DEFAULT_TESTDEF_SCRIPT))
> +
> +            with open(default_script, "w") as write_file:
> +                write_file.write(DEFAULT_TESTDEF_SCRIPT_CONTENT)

if this behaviour of "create a file with this content here" starts to repeat,
we probably want to encapsulatet it somewhere else (I noted it at least once in
other parts of this MP).

> +
> +            # Prompt the user to write the script file.
> +            self.edit_file(default_script)
> +
> +        # Make sure the script is executable.
> +        os.chmod(default_script,
> +                 stat.S_IRWXU | stat.S_IRWXG | stat.S_IROTH | stat.S_IXOTH)
> +
> +        print >> sys.stdout, ("\nCreating test definition "
> +                              "'{0}':".format(DEFAULT_TESTDEF_FILE))

print already writes to stdout by default. is it really necessary to be that explicit?

> +        self._create_test_file(os.path.join(test_path,
> +                                            DEFAULT_TESTDEF_FILE))
> +
> +        job = data[JOBFILE_ID]
> +        print >> sys.stdout, "\nCreating job file '{0}':".format(job)
> +        self._create_job_file(os.path.join(full_path, job), test_path)
> +
> +    def _create_job_file(self, job_file, test_path=None):
> +        """Creates the job file on the filesystem."""
> +
> +        # Invoke the command to create new job files: make a copy of the local
> +        # args and add what is necessary for the command.
> +        from lava.job.commands import new
> +
> +        args = copy.copy(self.args)
> +        args.FILE = job_file
> +
> +        job_cmd = new(self.parser, args)
> +        job_cmd.invoke(tests_dir=test_path)
> +
> +    def _create_test_file(self, test_file):
> +        """Creates the test definition file on the filesystem."""
> +
> +        # Invoke the command to create new testdef files: make a copy of the
> +        # local args and add what is necessary for the command.
> +        from lava.testdef.commands import new
> +
> +        args = copy.copy(self.args)
> +        args.FILE = test_file
> +
> +        testdef_cmd = new(self.parser, args)
> +        testdef_cmd.invoke()

I’m not sure about this approach of calling other commands. IMO the best
approach would be to concentrate as much of the logic as possible in the
model/backend classes, them call them from both commands.

> +class run(BaseCommand):
> +    """Runs a job on the local dispatcher."""
> +
> +    @classmethod
> +    def register_arguments(cls, parser):
> +        super(run, cls).register_arguments(parser)
> +        parser.add_argument("JOB",
> +                            help=("The job file to run, or a directory "
> +                                  "containing a job file. If nothing is "
> +                                  "passed, it uses the current working "
> +                                  "directory."),
> +                            nargs="?",
> +                            default=os.getcwd())
> +
> +    def invoke(self):
> +        full_path = os.path.abspath(self.args.JOB)
> +        job_file = self.retrieve_file(full_path, JOB_FILE_EXTENSIONS)
> +
> +        self._run_job(job_file)
> +
> +    def _run_job(self, job_file):
> +        """Runs a job on the dispatcher."""
> +        from lava.job.commands import run
> +
> +        args = copy.copy(self.args)
> +        args.FILE = job_file
> +
> +        run_cmd = run(self.parser, args)
> +        run_cmd.invoke()

ditto

> +class submit(BaseCommand):
> +    """Submits a job to LAVA."""
> +
> +    @classmethod
> +    def register_arguments(cls, parser):
> +        super(submit, cls).register_arguments(parser)
> +        parser.add_argument("JOB",
> +                            help=("The job file to send, or a directory "
> +                                  "containing a job file. If nothing is "
> +                                  "passed, it uses the current working "
> +                                  "directory."),
> +                            nargs="?",
> +                            default=os.getcwd())
> +
> +    def invoke(self):
> +        full_path = os.path.abspath(self.args.JOB)
> +        job_file = self.retrieve_file(full_path, JOB_FILE_EXTENSIONS)
> +
> +
> +    def _submit_job(self, job_file):
> +        """Submits a job file to LAVA."""
> +        from lava.job.commands import submit
> +
> +        args = copy.copy(self.args)
> +        args.FILE = job_file
> +
> +        submit_cmd = submit(self.parser, args)
> +        submit_cmd.invoke()

ditto

> +class update(BaseCommand):
> +    """Updates a job file with the correct data."""
> +
> +    @classmethod
> +    def register_arguments(cls, parser):
> +        super(update, cls).register_arguments(parser)
> +        parser.add_argument("JOB",
> +                            help=("Automatically updates a job file "
> +                                  "definition. If nothing is passed, it uses"
> +                                  "the current working directory."),
> +                            nargs="?",
> +                            default=os.getcwd())
> +
> +    def invoke(self):
> +        full_path = os.path.abspath(self.args.JOB)
> +        job_file = self.retrieve_file(full_path, JOB_FILE_EXTENSIONS)
> +        job_dir = os.path.dirname(job_file)
> +        tests_dir = os.path.join(job_dir, TESTS_DIR)
> +
> +        if os.path.isdir(tests_dir):
> +            encoded_tests = TarRepoParameter.get_encoded_tar(tests_dir)
> +
> +            json_data = None
> +            with open(job_file, "r") as json_file:
> +                try:
> +                    json_data = json.load(json_file)
> +                    set_value(json_data, TESTDEF_REPOS_TAR_REPO, encoded_tests)
> +                except Exception:
> +                    raise CommandError("Cannot read job file '{0}'.".format(
> +                        job_file))
> +
> +            with open(job_file, "w") as write_file:
> +                try:
> +                    write_file.write(json.dumps(json_data, indent=4))
> +                except Exception:
> +                    raise CommandError("Cannot update job file "
> +                                       "'{0}'.".format(json_file))
> +            print >> sys.stdout, "Job definition updated."
> +        else:
> +            raise CommandError("Cannot find tests directory.")

IIUC you are assuming that you will be always storing the tarball in the job
file? My initial thinking was to always generate a tarball dinamically, so that
you don’t have to explicitly update a job file. It would go like this:

$ lava job submit FOO.json

  submits FOO.json as ir

$ lava testdef submit FOO.yaml

  creates the tarball and a job file dinamically; submit those

$ lava script submit FOO.sh

  creates testdef, tarball and job file dynamically; submit them

> === modified file 'lava/config.py'
> --- lava/config.py	2013-07-22 09:32:25 +0000
> +++ lava/config.py	2013-07-22 09:32:26 +0000
> @@ -30,6 +30,7 @@
>      NoSectionError,
>  )
>  
> +from lava.parameter import Parameter
>  from lava.tool.errors import CommandError
>  
>  __all__ = ['Config', 'InteractiveConfig']
> @@ -54,14 +55,45 @@
>  atexit.register(_run_at_exit)
>  
>  
> +class Singleton(type):
> +    """A singleton implementation.
> +
> +    Configuration should be shared among the various call. The other approach
> +    would be to turn this into a module or a class with only static/class
> +    methods.
> +    """
> +    _instances = {}
> +
> +    def __call__(cls, *args, **kwargs):
> +        if cls not in cls._instances:
> +            cls._instances[cls] = super(Singleton, cls).__call__(*args,
> +                                                                 **kwargs)
> +        return cls._instances[cls]
> +
> +    @classmethod
> +    def _drop(cls):
> +        "Drop the instance (for testing purposes)."
> +        Singleton._instances = {}

what is the difference between using this Singleton implementation and just
instantiating a single object during initialization and exposing that object to
be used by other modules?

>  class Config(object):
>      """A generic config object."""
> -    def __init__(self):
> +    # Between all the various singleton Python patterns, the metaclass one
> +    # looks like is the one that works better with inheritance.
> +    # In Python3 this would go into the class declaration as follows:
> +    # class Config(object, metaclass=Singleton)
> +    __metaclass__ = Singleton
> +
> +    def __init__(self, config_file=None):
>          # The cache where to store parameters.
>          self._cache = {}
> -        self._config_file = (os.environ.get('LAVACONFIG') or
> -                             os.path.join(os.path.expanduser('~'),
> -                                          '.lavaconfig'))
> +        # Mostly needed for testing purposes.
> +        if config_file is not None:
> +            self._config_file = config_file
> +        else:
> +            self._config_file = (os.environ.get('LAVACONFIG') or
> +                                 os.path.join(os.path.expanduser('~'),
> +                                              '.lavaconfig'))

you could instead turn config_file into a property with lazy initialization,
then for testing you could mock that method to make it return the config data
you want.

>          self._config_backend = ConfigParser()
>          self._config_backend.read([self._config_file])
>          AT_EXIT_CALLS.add(self.save)
> @@ -92,7 +124,7 @@
>          if not section:
>              section = self._calculate_config_section(parameter)
>          # Try to get the parameter value first if it has one.
> -        if parameter.value:
> +        if parameter.value is not None:
>              value = parameter.value
>          else:
>              value = self._get_from_cache(parameter, section)
> @@ -150,6 +182,13 @@
>          if (not self._config_backend.has_section(section) and
>                  section != DEFAULT_SECTION):
>              self._config_backend.add_section(section)
> +
> +        # This is done to serialize a list when ConfigParser is written to
> +        # file. Since there is no real support for list in ConfigParser, we
> +        # serialized it in a common way that can get easily deserialized.
> +        if isinstance(value, list):
> +            value = Parameter.serialize(value)
> +
>          self._config_backend.set(section, key, value)
>          # Store in the cache too.
>          self._put_in_cache(key, value, section)
> @@ -158,6 +197,7 @@
>          """Adds a Parameter to the config file and cache.
>  
>          :param Parameter: The parameter to add.
> +        :type Parameter
>          :param value: The value of the parameter. Defaults to None.
>          :param section: The section where this parameter should be stored.
>                          Defaults to None.
> @@ -184,8 +224,8 @@
>      If a value is not found in the config file, it will ask it and then stores
>      it.
>      """
> -    def __init__(self, force_interactive=True):
> -        super(InteractiveConfig, self).__init__()
> +    def __init__(self, config_file=None, force_interactive=True):
> +        super(InteractiveConfig, self).__init__(config_file=config_file)
>          self._force_interactive = force_interactive
>  
>      def get(self, parameter, section=None):
> @@ -198,10 +238,9 @@
>              section = self._calculate_config_section(parameter)
>          value = super(InteractiveConfig, self).get(parameter, section)
>  
> -        if not (value is not None and parameter.asked):
> -            if not value or self._force_interactive:
> -                value = parameter.prompt(old_value=value)
> +        if value is None or self._force_interactive:
> +            value = parameter.prompt(old_value=value)
>  
> -        if value is not None:
> +        if value is not None and parameter.store:
>              self.put(parameter.id, value, section)
>          return value
> 
> === modified file 'lava/helper/command.py'
> --- lava/helper/command.py	2013-07-22 09:32:25 +0000
> +++ lava/helper/command.py	2013-07-22 09:32:26 +0000
> @@ -21,7 +21,7 @@
>  import os
>  import subprocess
>  import sys
> -
> +import types
>  
>  from lava.config import InteractiveConfig
>  from lava.tool.command import Command
> @@ -30,7 +30,9 @@
>  
>  
>  class BaseCommand(Command):
> +
>      """Base command class for all lava commands."""
> +
>      def __init__(self, parser, args):
>          super(BaseCommand, self).__init__(parser, args)
>          self.config = InteractiveConfig(
> @@ -59,10 +61,10 @@
>          return can_edit
>  
>      @classmethod
> -    def edit_file(cls, config_file):
> +    def edit_file(cls, file_to_edit):
>          """Opens the specified file with the default file editor.
>  
> -        :param config_file: The file to edit.
> +        :param file_to_edit: The file to edit.
>          """
>          editor = os.environ.get("EDITOR", None)
>          if editor is None:
> @@ -73,27 +75,71 @@
>              else:
>                  # We really do not know how to open a file.
>                  print >> sys.stdout, ("Cannot find an editor to open the "
> -                                      "file '{0}'.".format(config_file))
> +                                      "file '{0}'.".format(file_to_edit))
>                  print >> sys.stdout, ("Either set the 'EDITOR' environment "
>                                        "variable, or install 'sensible-editor' "
>                                        "or 'xdg-open'.")
>                  sys.exit(-1)
>          try:
> -            subprocess.Popen([editor, config_file]).wait()
> +            subprocess.Popen([editor, file_to_edit]).wait()
>          except Exception:
>              raise CommandError("Error opening the file '{0}' with the "
> -                               "following editor: {1}.".format(config_file,
> +                               "following editor: {1}.".format(file_to_edit,
>                                                                 editor))
>  
>      @classmethod
> +    def retrieve_file(cls, path, extensions):
> +        """Searches for a job file in the provided path.
> +
> +        :return The job file full path.
> +        """

it look to me that this method could be generic. If that doesn’t make sense, it
could probably be called `find_job_file` or something like that.

> +        if os.path.isfile(path):
> +            job_file = path
> +        else:
> +            for element in os.listdir(path):
> +                element_path = os.path.join(path, element)
> +                if os.path.isdir(element_path):
> +                    continue
> +                elif os.path.isfile(element_path):
> +                    job_file = os.path.split(element)[1]
> +                    # Extension here contains also the leading dot.
> +                    full_extension = os.path.splitext(job_file)[1]
> +
> +                    if full_extension:
> +                        # Make sure that we have an extension and remove the
> +                        # dot.
> +                        extension = full_extension[1:].strip().lower()
> +                        if extension in extensions:
> +                            job_file = element_path
> +                            break
> +            else:
> +                raise CommandError("No job file found in: '{0}'".format(path))
> +
> +        return job_file
> +
> +    @classmethod
> +    def verify_file_extension(cls, path, default, supported):
> +        # Checks that the file we pass has an extension or a correct one.
> +        # If not, it adds the provided one.

this comment is rather confusing

> +        full_path, file_name = os.path.split(path)
> +        name, extension = os.path.splitext(file_name)
> +        if not extension:
> +            path = ".".join([path, default])
> +        elif extension[1:] not in supported:
> +            path = os.path.join(full_path, ".".join([name, default]))
> +        return path
> +
> +    @classmethod
>      def run(cls, cmd_args):
>          """Runs the supplied command args.
>  
>          :param cmd_args: The command, and its optional arguments, to run.
>          :return The command execution return code.
>          """
> -        if not isinstance(cmd_args, list):
> +        if isinstance(cmd_args, types.StringTypes):
>              cmd_args = [cmd_args]
> +        else:
> +            cmd_args = list(cmd_args)
>          try:
>              return subprocess.check_call(cmd_args)
>          except subprocess.CalledProcessError:
> 
> === modified file 'lava/helper/template.py'
> --- lava/helper/template.py	2013-07-22 09:32:25 +0000
> +++ lava/helper/template.py	2013-07-22 09:32:26 +0000
> @@ -16,6 +16,8 @@
>  # 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/>.
>  
> +"""Helper functions for a template."""
> +
>  from lava.parameter import Parameter
>  
>  
> @@ -42,3 +44,76 @@
>                  update(entry)
>  
>      update(template)
> +
> +
> +def get_key(data, search_key):
> +    """Goes through a template looking for a key.
> +
> +    :param data: The template to traverse.
> +    :param search_key: The key to look for.
> +    :return The key value.
> +    """
> +    return_value = None
> +
> +    if isinstance(data, dict):
> +        bucket = []
> +
> +        for key, value in data.iteritems():
> +            if key == search_key:
> +                return_value = value
> +                break
> +            else:
> +                bucket.append(value)
> +
> +        if bucket and return_value is None:
> +            for element in bucket:
> +                if isinstance(element, list):
> +                    for element in element:
> +                        bucket.append(element)
> +                elif isinstance(element, dict):
> +                    for key, value in element.iteritems():
> +                        if key == search_key:
> +                            return_value = value
> +                            break
> +                        else:
> +                            bucket.append(value)
> +
> +    return return_value
> +
> +
> +def set_value(data, search_key, new_value):
> +    """Sets a new value for a template key.
> +
> +    :param data: The data structure to update.
> +    :type dict
> +    :param search_key: The key to search and update.
> +    :param new_value: The new value to set.
> +    """
> +    is_set = False
> +
> +    if isinstance(data, dict):
> +        bucket = []
> +
> +        for key, value in data.iteritems():
> +            if key == search_key:
> +                data[key] = new_value
> +                is_set = True
> +                break
> +            else:
> +                bucket.append(value)
> +
> +        if bucket and not is_set:
> +            for element in bucket:
> +                if isinstance(element, list):
> +                    for element in element:
> +                        bucket.append(element)
> +                elif isinstance(element, dict):
> +                    for key, value in element.iteritems():
> +                        if key == search_key:
> +                            element[key] = new_value
> +                            is_set = True
> +                            break
> +                        else:
> +                            bucket.append(value)
> +                if is_set:
> +                    break
> 
> === modified file 'lava/helper/tests/helper_test.py'
> --- lava/helper/tests/helper_test.py	2013-07-22 09:32:25 +0000
> +++ lava/helper/tests/helper_test.py	2013-07-22 09:32:26 +0000
> @@ -29,12 +29,20 @@
>  import tempfile
>  
>  from unittest import TestCase
> -from mock import MagicMock
> +from mock import (
> +    MagicMock,
> +    patch
> +)
>  
>  
>  class HelperTest(TestCase):
>      """Helper test class that all tests under the lava package can inherit."""
> +
>      def setUp(self):
> +        # Need to patch it here, not as a decorator, or running the tests
> +        # via `./setup.py test` will fail.
> +        self.at_exit_patcher = patch("lava.config.AT_EXIT_CALLS", spec=set)
> +        self.at_exit_patcher.start()
>          self.original_stdout = sys.stdout
>          sys.stdout = open("/dev/null", "w")
>          self.original_stderr = sys.stderr
> @@ -50,12 +58,18 @@
>          self.args.interactive = MagicMock(return_value=False)
>          self.args.DEVICE = self.device
>  
> -        self.config = MagicMock()
> -        self.config.get = MagicMock(return_value=self.temp_dir)
> -
>      def tearDown(self):
> +        self.at_exit_patcher.stop()
>          sys.stdin = self.original_stdin
>          sys.stdout = self.original_stdout
>          sys.stderr = self.original_stderr
>          shutil.rmtree(self.temp_dir)
>          os.unlink(self.temp_file.name)
> +
> +    def tmp(self, name):
> +        """Returns a path to a non existent file/directory.

A method called `tmp` that returns a non existing file is rather confusing. Can
we come up with a better name? Maybe the method name is correct but the
docstring needs to say something less confusing.

What about

"""
Returns the full path to a file called `name` in a temporary directory. This
method does not create the file, it only gives a full filename where you can
actually write some data.
"""

> +
> +        :param name: The name the file/directory should have.
> +        :return A path.
> +        """
> +        return os.path.join(tempfile.gettempdir(), name)
> 
> === modified file 'lava/job/commands.py'
> --- lava/job/commands.py	2013-07-22 09:32:25 +0000
> +++ lava/job/commands.py	2013-07-22 09:32:26 +0000
> @@ -26,18 +26,36 @@
>  
>  from lava.helper.command import BaseCommand
>  from lava.helper.dispatcher import get_devices
> -
> +from lava.helper.template import get_key
>  from lava.job import Job
>  from lava.job.templates import (
> -    BOOT_TEST,
> -)
> -from lava.parameter import Parameter
> +    LAVA_TEST_SHELL,
> +    TESTDEF_REPOS_TAR_REPO,
> +)
> +from lava.parameter import (
> +    ListParameter,
> +    Parameter,
> +    SingleChoiceParameter,
> +)
>  from lava.tool.command import CommandGroup
>  from lava.tool.errors import CommandError
>  from lava_tool.authtoken import AuthenticatingServerProxy, KeyringAuthBackend
>  from lava_tool.utils import has_command
>  
>  
> +# Name of the config value to store the job ids.
> +JOBS_ID = "jobs_id"
> +# Name of the config value to store the LAVA server URL.
> +SERVER = "server"
> +# Name of the config value to store the LAVA rpc_endpoint.
> +RPC_ENDPOINT = "rpc_endpoint"
> +
> +# Default job file extension.
> +DEFAULT_EXTENSION = "json"
> +# Possible extension for a job file.
> +JOB_FILE_EXTENSIONS = [DEFAULT_EXTENSION]
> +
> +
>  class job(CommandGroup):
>      """LAVA job file handling."""
>      namespace = 'lava.job.commands'
> @@ -51,19 +69,31 @@
>          super(new, cls).register_arguments(parser)
>          parser.add_argument("FILE", help=("Job file to be created."))
>  
> -    def invoke(self):
> -        if os.path.exists(self.args.FILE):
> -            raise CommandError('{0} already exists.'.format(self.args.FILE))
> -
> -        with open(self.args.FILE, 'w') as job_file:
> -            job_instance = Job(BOOT_TEST)
> +    def invoke(self, tests_dir=None):
> +        full_path = os.path.abspath(self.args.FILE)
> +        job_file = self.verify_file_extension(full_path, DEFAULT_EXTENSION,
> +                                              JOB_FILE_EXTENSIONS)
> +
> +        if os.path.exists(job_file):
> +            raise CommandError('{0} already exists.'.format(job_file))
> +
> +        job_instance = Job(LAVA_TEST_SHELL)
> +        if tests_dir:
> +            # TODO: find a better way to retrieve a key.
> +            testdef_tar_repo = get_key(TESTDEF_REPOS_TAR_REPO)
> +            testdef_tar_repo.set(tests_dir)
> +            testdef_tar_repo.asked = True
> +
> +        with open(job_file, 'w') as write_file:
>              job_instance.fill_in(self.config)
> -            job_instance.write(job_file)
> +            job_instance.write(write_file)
>  
>  
>  class submit(BaseCommand):
>      """Submits the specified job file."""
>  
> +    JOBS_ID = "jobs_id"
> +
>      @classmethod
>      def register_arguments(cls, parser):
>          super(submit, cls).register_arguments(parser)
> @@ -73,16 +103,31 @@
>          jobfile = self.args.FILE
>          jobdata = open(jobfile, 'rb').read()
>  
> -        server_name = Parameter('server')
> -        rpc_endpoint = Parameter('rpc_endpoint', depends=server_name)
> -        self.config.get(server_name)
> -        endpoint = self.config.get(rpc_endpoint)
> +        server_name_parameter = Parameter(SERVER)
> +        rpc_endpoint_parameter = Parameter(RPC_ENDPOINT,
> +                                           depends=server_name_parameter)
> +        self.config.get(server_name_parameter)
> +        endpoint = self.config.get(rpc_endpoint_parameter)
>  
>          server = AuthenticatingServerProxy(endpoint,
>                                             auth_backend=KeyringAuthBackend())
>          try:
>              job_id = server.scheduler.submit_job(jobdata)
>              print >> sys.stdout, "Job submitted with job ID {0}".format(job_id)
> +
> +            # Store the job_id into the config file.
> +            if job_id:
> +                # We need first to take out the old values, and then store the
> +                # new one.
> +                job_id_parameter = ListParameter(JOBS_ID)
> +                job_id_parameter.asked = True
> +
> +                value = self.config.get(job_id_parameter)
> +                if value:
> +                    job_id_parameter.set(value)
> +
> +                job_id_parameter.add(job_id)
> +                self.config.put_parameter(job_id_parameter)
>          except xmlrpclib.Fault, exc:
>              raise CommandError(str(exc))
>  

I don't think we should keep the list of submitted jobs in the config file.
Think of someone that wants to keep their lava config file under version
control (I do that for most of all my config files); that user won't want
lava-too to change the config file for every job submission.

Instead, I think the list of job ids (and eventually some extra data about each
job) is runtime data, and as such should be kept under ~/.cache/lava/jobs or
something like that

> @@ -95,42 +140,16 @@
>          super(run, cls).register_arguments(parser)
>          parser.add_argument("FILE", help=("The job file to submit."))
>  
> -    @classmethod
> -    def _choose_device(cls, devices):
> -        """Let the user choose the device to use.
> -
> -        :param devices: The list of available devices.
> -        :return The selected device.
> -        """
> -        devices_len = len(devices)
> -        output_list = []
> -        for device, number in zip(devices, range(1, devices_len + 1)):
> -            output_list.append("\t{0}. {1}\n".format(number, device.hostname))
> -
> -        print >> sys.stdout, ("More than one local device found. "
> -                              "Please choose one:\n")
> -        print >> sys.stdout, "".join(output_list)
> -
> -        while True:
> -            try:
> -                user_input = raw_input("Device number to use: ").strip()
> -
> -                if user_input in [str(x) for x in range(1, devices_len + 1)]:
> -                    return devices[int(user_input) - 1].hostname
> -                else:
> -                    continue
> -            except EOFError:
> -                user_input = None
> -            except KeyboardInterrupt:
> -                sys.exit(-1)
> -
>      def invoke(self):
>          if os.path.isfile(self.args.FILE):
>              if has_command("lava-dispatch"):
>                  devices = get_devices()
>                  if devices:
>                      if len(devices) > 1:
> -                        device = self._choose_device(devices)
> +                        device_names = [device.hostname for device in devices]
> +                        device_param = SingleChoiceParameter("device",
> +                                                             device_names)
> +                        device = device_param.prompt("Device to use: ")
>                      else:
>                          device = devices[0].hostname
>                      self.run(["lava-dispatch", "--target", device,
> @@ -138,5 +157,5 @@
>              else:
>                  raise CommandError("Cannot find lava-dispatcher installation.")
>          else:
> -            raise CommandError("The file '{0}' does not exists. or is not "
> +            raise CommandError("The file '{0}' does not exists, or it is not "
>                                 "a file.".format(self.args.FILE))
> 
> === modified file 'lava/job/templates.py'
> --- lava/job/templates.py	2013-07-22 09:32:25 +0000
> +++ lava/job/templates.py	2013-07-22 09:32:26 +0000
> @@ -16,44 +16,78 @@
>  # 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/>.
>  
> -from lava.parameter import Parameter
> -
> -device_type = Parameter("device_type")
> -prebuilt_image = Parameter("prebuilt_image", depends=device_type)
> +from lava.parameter import (
> +    Parameter,
> +    TarRepoParameter,
> +)
> +
> +ACTIONS_ID = "actions"
> +COMMAND_ID = "command"
> +IMAGE_ID = "image"
> +PARAMETERS_ID = "parameters"
> +DEVICE_TYPE_ID = "device_type"
> +TESTDEF_REPOS_ID = "testdef_repos"
> +TESTDEF_REPOS_TAR_REPO = "tar-repo"
> +STREAM_ID = "stream"
> +SERVER_ID = "server"

I don’t think this many constants are usefull. I would rather expect the actual
template data to be as close to the actual data as possible and believe that
will make their maintaince easier.

> +
> +DEVICE_TYPE_PARAMETER = Parameter(DEVICE_TYPE_ID)
> +PREBUILT_IMAGE_PARAMETER = Parameter(IMAGE_ID, depends=DEVICE_TYPE_PARAMETER)
> +
> +# Never store the testdef_urls parameter in the config file.
> +TESTDEF_URL_PARAMETER = TarRepoParameter(TESTDEF_REPOS_TAR_REPO)
> +TESTDEF_URL_PARAMETER.store = False
> +
> +# Use another ID for the server parameter, might be different.
> +SERVER_PARAMETER = Parameter("stream_server")
> +STREAM_PARAMETER = Parameter(STREAM_ID)
>  
>  BOOT_TEST = {
> +    "timeout": 18000,
>      "job_name": "Boot test",
> -    "device_type": device_type,
> -    "actions": [
> +    DEVICE_TYPE_ID: DEVICE_TYPE_PARAMETER,
> +    ACTIONS_ID: [
>          {
> -            "command": "deploy_linaro_image",
> -            "parameters": {
> -                "image": prebuilt_image
> +            COMMAND_ID: "deploy_linaro_image",
> +            PARAMETERS_ID: {
> +                "image": PREBUILT_IMAGE_PARAMETER
>              }
>          },
>          {
> -            "command": "boot_linaro_image"
> +            COMMAND_ID: "boot_linaro_image"
>          }
>      ]
>  }
>  
>  LAVA_TEST_SHELL = {
>      "job_name": "LAVA Test Shell",
> -    "device_type": device_type,
> -    "actions": [
> +    "timeout": 18000,
> +    DEVICE_TYPE_ID: DEVICE_TYPE_PARAMETER,
> +    ACTIONS_ID: [
>          {
> -            "command": "deploy_linaro_image",
> -            "parameters": {
> -                "image": prebuilt_image,
> +            COMMAND_ID: "deploy_linaro_image",
> +            PARAMETERS_ID: {
> +                IMAGE_ID: PREBUILT_IMAGE_PARAMETER,
>              }
>          },
>          {
> -            "command": "lava_test_shell",
> -            "parameters": {
> -                "testdef_urls": [
> -                    Parameter("testdef_url")
> +            COMMAND_ID: "lava_test_shell",
> +            PARAMETERS_ID: {
> +                "timeout": 1800,
> +                TESTDEF_REPOS_ID: [
> +                        {
> +                            "testdef": "lavatest.yaml",
> +                            TESTDEF_REPOS_TAR_REPO: TESTDEF_URL_PARAMETER
> +                        }
>                  ]
>              }
> +        },
> +        {
> +            COMMAND_ID: "submit_results",
> +            PARAMETERS_ID : {
> +                STREAM_ID: STREAM_PARAMETER,
> +                SERVER_ID: SERVER_PARAMETER
> +            }
>          }
>      ]
>  }
> 
> === modified file 'lava/parameter.py'
> --- lava/parameter.py	2013-07-22 09:32:25 +0000
> +++ lava/parameter.py	2013-07-22 09:32:26 +0000
> @@ -20,7 +20,19 @@
>  Parameter class and its accessory methods/functions.
>  """
>  
> +import StringIO
> +import base64
> +import os
>  import sys
> +import tarfile
> +import tempfile
> +import types
> +import urlparse
> +
> +from lava.tool.errors import CommandError
> +
> +# Character used to join serialized list parameters.
> +LIST_SERIALIZE_DELIMITER = ","
>  
>  
>  class Parameter(object):
> @@ -38,6 +50,15 @@
>          self.value = value
>          self.depends = depends
>          self.asked = False
> +        # Whether to store or not the parameter in the user config file.
> +        self.store = True
> +
> +    def set(self, value):
> +        """Sets the value of the parameter.
> +
> +        :param value: The value to set.
> +        """
> +        self.value = value
>  
>      def prompt(self, old_value=None):
>          """Gets the parameter value from the user.
> @@ -50,26 +71,287 @@
>          :param old_value: The old parameter value.
>          :return The input as typed by the user, or the old value.
>          """
> -        if old_value is not None:
> -            prompt = "{0} [{1}]: ".format(self.id, old_value)
> -        else:
> -            prompt = "{0}: ".format(self.id)
> -
> -        user_input = None
> +        if not self.asked:
> +            if old_value is not None:
> +                prompt = "{0} [{1}]: ".format(self.id, old_value)
> +            else:
> +                prompt = "{0}: ".format(self.id)
> +
> +            user_input = self.get_user_input(prompt)
> +
> +            if user_input is not None:
> +                if len(user_input) == 0 and old_value:
> +                    # Keep the old value when user press enter or another
> +                    # whitespace char.
> +                    self.value = old_value
> +                else:
> +                    self.value = user_input
> +
> +            self.asked = True
> +
> +        return self.value
> +
> +    @classmethod
> +    def get_user_input(cls, prompt=""):
> +        """Asks the user for input data.
> +
> +        :param prompt: The prompt that should be given to the user.
> +        :return A string with what the user typed.
> +        """
> +        data = None
>          try:
> -            user_input = raw_input(prompt).strip()
> +            data = raw_input(prompt).strip()
>          except EOFError:
> -            pass
> +            # Force to return None.
> +            data = None
>          except KeyboardInterrupt:
>              sys.exit(-1)
> -
> -        if user_input is not None:
> +        return data
> +
> +    @classmethod
> +    def serialize(cls, value):
> +        """Serializes the passed value to be friendly written to file.
> +
> +        Lists are serialized as a comma separated string of values.

I already saw something about serializing lists above. Can we keep all this
list serialization/deserialization just here?

> +
> +        :param value: The value to serialize.
> +        :return The serialized value as string.
> +        """
> +        serialized = ""
> +        if isinstance(value, list):
> +            serialized = LIST_SERIALIZE_DELIMITER.join(
> +                str(x) for x in value if x)
> +        else:
> +            serialized = str(value)
> +        return serialized

I wonder if we can we use a existing serialization instead of using a arbitrary
delimiter ...  e.g. json + base64, this way we don’t have to limit ourselves to
values that do not contain a comma. But OTOH we want something that is readable
(and editable!) in the config file ...

> +    @classmethod
> +    def deserialize(cls, value):
> +        """Deserialize a value into a list.
> +
> +        The value must have been serialized with the class instance serialize()
> +        method.
> +
> +        :param value: The string value to be deserialized.
> +        :type str
> +        :return A list of values.
> +        """
> +        deserialized = []
> +        if isinstance(value, types.StringTypes):
> +            deserialized = filter(None, (x.strip() for x in value.split(
> +                LIST_SERIALIZE_DELIMITER)))
> +        else:
> +            deserialized = list(value)
> +        return deserialized
> +
> +    @classmethod
> +    def to_list(cls, value):
> +        """Return a list from the passed value.
> +
> +        :param value: The parameter to turn into a list.
> +        """
> +        return_value = []
> +        if isinstance(value, types.StringType):
> +            return_value = [value]
> +        else:
> +            return_value = list(value)
> +        return return_value
> +
> +
> +class SingleChoiceParameter(Parameter):
> +    """A parameter implemeting a single choice between multiple choices."""
> +    def __init__(self, id, choices):
> +        super(SingleChoiceParameter, self).__init__(id)
> +        self.choices = self.to_list(choices)
> +
> +    def prompt(self, prompt, old_value=None):
> +        """Asks the user for their choice."""
> +        # Sliglty different than the other parameters: here we first present
> +        # the user with what the choices are about.
> +        print >> sys.stdout, prompt
> +
> +        index = 1
> +        for choice in self.choices:
> +            print >> sys.stdout, "\t{0:d}. {1}".format(index, choice)
> +            index += 1
> +
> +        choices_len = len(self.choices)
> +        while True:
> +            user_input = self.get_user_input("Choice: ")
> +
>              if len(user_input) == 0 and old_value:
> -                # Keep the old value when user press enter or another
> -                # whitespace char.
> -                self.value = old_value
> -            else:
> -                self.value = user_input
> -
> -        self.asked = True
> -        return self.value
> +                choice = old_value
> +                break
> +            elif user_input in [str(x) for x in range(1, choices_len + 1)]:
> +                choice = self.choices[int(user_input) - 1]
> +                break
> +
> +        return choice
> +
> +
> +class UrlSchemeParameter(SingleChoiceParameter):
> +    """A parameter to choose the URL scheme to use."""
> +    # The supported URL schemes:
> +    #   file: normal file URL.
> +    #   data: base64 encoded string of the file pointed to and its path.
> +    FILE_SCHEME = "file"
> +    DATA_SCHEME = "data"
> +
> +    SCHEME_CHOICES = [
> +        FILE_SCHEME,
> +        DATA_SCHEME,
> +    ]
> +
> +    def __init__(self, value=None, id="url_scheme"):
> +        super(UrlSchemeParameter, self).__init__(id, self.SCHEME_CHOICES)
> +        self.value = value
> +
> +    def prompt(self, prompt="", old_value=None):
> +        if not self.asked:
> +            if old_value is not None:
> +                prompt = "\nURL scheme [was: {0}]:".format(old_value)
> +            else:
> +                prompt = "\nURL scheme:"
> +            self.value = super(UrlSchemeParameter, self).prompt(
> +                prompt=prompt, old_value=old_value)
> +            self.asked = True
> +
> +        return self.value

As far as this MP is concerned this class is not used (apart from its own unit
tests). This is the one you mentioned in the introduction right?

I can't see the point of a URL scheme parameter, I think it's easier to just
ask for the URL itself and then deduce the scheme from there. See also my
comment above about the tarball (i.e. you should never need to inform the
tarball data manually)

> +class ListParameter(Parameter):
> +    """A specialized Parameter to handle list values."""
> +
> +    # This is used as a deletion character. When we have an old value and the
> +    # user enters this char, it sort of deletes the value.
> +    DELETE_CHAR = "-"
> +
> +    def __init__(self, id, value=None, depends=None):
> +        super(ListParameter, self).__init__(id, depends=depends)
> +        self.value = []
> +        if value:
> +            self.set(value)
> +
> +    def set(self, value):
> +        """Sets the value of the parameter.
> +
> +        :param value: The value to set.
> +        """
> +        self.value = self.to_list(value)
> +
> +    def add(self, value):
> +        """Adds a new value to the list of values of this parameter.
> +
> +        :param value: The value to add.
> +        """
> +        if isinstance(value, list):
> +            self.value.extend(value)
> +        else:
> +            self.value.append(value)
> +
> +    def prompt(self, old_value=None):
> +        """Gets the parameter in a list form.
> +
> +        To exit the input procedure it is necessary to insert an empty line.
> +
> +        :return The list of values.
> +        """
> +
> +        if not self.asked:
> +            if old_value is not None:
> +                # We might get the old value read from file via ConfigParser,
> +                # and usually it comes in string format.
> +                old_value = self.deserialize(old_value)
> +
> +            print >> sys.stdout, "Values for '{0}': ".format(self.id)
> +
> +            index = 1
> +            while True:
> +                user_input = None
> +                if old_value is not None and (0 < len(old_value) >= index):
> +                    prompt = "{0:>3d}.\n\told: {1}\n\tnew: ".format(
> +                        index, old_value[index-1])
> +                    user_input = self.get_user_input(prompt)
> +                else:
> +                    prompt = "{0:>3d}. ".format(index)
> +                    user_input = self.get_user_input(prompt)
> +
> +                if user_input is not None:
> +                    # The user has pressed Enter.
> +                    if len(user_input) == 0:
> +                        if old_value is not None and \
> +                                (0 < len(old_value) >= index):
> +                            user_input = old_value[index-1]
> +                        else:
> +                            break
> +
> +                    if len(user_input) == 1 and user_input == \
> +                            self.DELETE_CHAR and (0 < len(old_value) >= index):
> +                        # We have an old value, user presses the DELETE_CHAR
> +                        # and we do not store anything. This is done to delete
> +                        # an old entry.
> +                        pass
> +                    else:
> +                        self.value.append(user_input)
> +                    index += 1
> +
> +            self.asked = True
> +
> +        return self.value
> +
> +
> +class TarRepoParameter(Parameter):
> +    def __init__(self, id="tar_repo_dir", value=None, depends=None):
> +        super(TarRepoParameter, self).__init__(id, value=value,
> +                                               depends=depends)
> +
> +    @classmethod
> +    def get_encoded_tar(cls, paths):
> +        """Encodes in base64 the provided path.
> +
> +        The econding will create a temporary tar file, read the content of
> +        the tarball and encode it in base64. At the end the temporary file
> +        will be deleted.
> +
> +        :param paths: Paths that will be added to the temporary tar file.
> +        :return The encoded content of the tar file.
> +        """
> +        paths = cls.to_list(paths)
> +        try:
> +            temp_tar_file = tempfile.NamedTemporaryFile(suffix=".tar",
> +                                                        delete=False)
> +            with tarfile.open(temp_tar_file.name, "w") as tar_file:
> +                for path in paths:
> +                    full_path = os.path.abspath(path)
> +                    if os.path.isfile(full_path):
> +                        arcname = os.path.basename(full_path)
> +                        tar_file.add(full_path, arcname=arcname)
> +                    elif os.path.isdir(full_path):
> +                        # If we pass a directory, flatten it out.
> +                        # List its content, and add them as they are.
> +                        for element in os.listdir(full_path):
> +                            arcname = element
> +                            tar_file.add(os.path.join(full_path, element),
> +                                         arcname=arcname)
> +
> +            encoded_content = StringIO.StringIO()
> +
> +            if os.path.isfile(temp_tar_file.name):
> +                try:
> +                    with open(temp_tar_file.name) as read_file:
> +                        base64.encode(read_file, encoded_content)
> +
> +                    return encoded_content.getvalue().strip()
> +                except IOError:
> +                    raise CommandError("Cannot read file "
> +                                       "'{0}'.".format(temp_tar_file.name))
> +            else:
> +                raise CommandError("Provided path does not exists: "
> +                                   "{0}.".format(temp_tar_file.name))
> +        finally:
> +            if os.path.isfile(temp_tar_file.name):
> +                os.unlink(temp_tar_file.name)
> +
> +    def prompt(self, old_value=None):
> +        super(TarRepoParameter, self).prompt(old_value=old_value)
> +        return self.get_encoded_tar(self.value)

As I commented above, this TarRepoParameter class should not be needed (i.e.
users should not need to deal with tarballs themselves)

> === added file 'lava/testdef/commands.py'
> --- lava/testdef/commands.py	1970-01-01 00:00:00 +0000
> +++ lava/testdef/commands.py	2013-07-22 09:32:26 +0000
> @@ -0,0 +1,93 @@
> +# 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/>.
> +
> +"""
> +Test definition commands class.
> +"""
> +
> +import os
> +
> +from lava.helper.command import BaseCommand
> +from lava.testdef import TestDefinition
> +from lava.testdef.templates import TESTDEF_TEMPLATE
> +from lava.tool.command import CommandGroup
> +from lava.tool.errors import CommandError
> +
> +
> +# Default test def file extension.
> +DEFAULT_TEST_EXTENSION = "yaml"
> +# Possible extensions for a test def file.
> +TEST_FILE_EXTENSIONS = [DEFAULT_TEST_EXTENSION]
> +
> +
> +class testdef(CommandGroup):
> +
> +    """LAVA test definitions handling."""
> +
> +    namespace = "lava.testdef.commands"
> +
> +
> +class new(BaseCommand):
> +
> +    """Creates a new test definition file."""
> +
> +    @classmethod
> +    def register_arguments(cls, parser):
> +        super(new, cls).register_arguments(parser)
> +        parser.add_argument("FILE", help="Test definition file to create.")
> +
> +    def invoke(self):
> +        full_path = os.path.abspath(self.args.FILE)
> +        testdef_file = self.verify_file_extension(full_path,
> +                                                  DEFAULT_TEST_EXTENSION,
> +                                                  TEST_FILE_EXTENSIONS)
> +        if os.path.exists(testdef_file):
> +            raise CommandError("Test definition file '{0}' already "
> +                               "exists.".format(self.args.FILE))
> +        try:
> +            testdef = TestDefinition(testdef_file, TESTDEF_TEMPLATE)
> +            testdef.update(self.config)
> +            testdef.write()
> +        except IOError:
> +            raise CommandError("Cannot write file '{0}': permission "
> +                               "denied".format(self.args.FILE))

I don't think that insuficient permissions are the only possible cause of an
IOError here ...

> +
> +
> +class run(BaseCommand):
> +
> +    """Runs the specified test definition on a local device."""
> +
> +    @classmethod
> +    def register_arguments(cls, parser):
> +        super(run, cls).register_arguments(parser)
> +        parser.add_argument("FILE", help="Test definition file to run.")
> +
> +    def invoke(self):
> +        pass

missing a FIXME/TODO comment? (I realize that this is probably coming in the
                               next MP though :))

> +def submit(BaseCommand):
> +    """Submits the specified test definition to a remove LAVA server."""
> +
> +    @classmethod
> +    def register_arguments(cls, parser):
> +        super(submit, cls).register_arguments(parser)
> +        parser.add_argument("FILE", help="Test definition file to send.")
> +
> +    def invoke(self):
> +        pass

ditto

> === added file 'lava/testdef/templates.py'
> --- lava/testdef/templates.py	1970-01-01 00:00:00 +0000
> +++ lava/testdef/templates.py	2013-07-22 09:32:26 +0000
> @@ -0,0 +1,75 @@
> +# 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/>.
> +
> +"""Test definition templates."""
> +
> +from lava.parameter import (
> +    ListParameter,
> +    Parameter,
> +)
> +
> +DEFAULT_TESTDEF_FILE = "lavatest.yaml"
> +
> +DEFAULT_TESTDEF_VERSION = "1.0"
> +DEFAULT_TESTDEF_FORMAT = "Lava-Test Test Definition 1.0"
> +
> +# This is what will be called by default by the test definition yaml file.
> +DEFAULT_TESTDEF_SCRIPT = "mytest.sh"
> +DEFAULT_TESTDEF_SCRIPT_CONTENT = """#!/bin/sh
> +# Automatic generated content by lava-tool.
> +# Please add your own instructions.
> +"""

It would be nice to include some commented examples of the lava-test-shell API
usage ...

> +DEFAULT_TESTDEF_STEP = "./mytest.sh"
> +
> +DEFAULT_ENVIRONMET_VALUE = "lava_test_shell"
> +
> +# All these parameters will not be stored on the local config file.
> +NAME_PARAMETER = Parameter("name")
> +NAME_PARAMETER.store = False
> +
> +DESCRIPTION_PARAMETER = Parameter("description", depends=NAME_PARAMETER)
> +DESCRIPTION_PARAMETER.store = False
> +
> +ENVIRONMENT_PARAMETER = ListParameter("environment",
> +                                      depends=NAME_PARAMETER)
> +ENVIRONMENT_PARAMETER.add(DEFAULT_ENVIRONMET_VALUE)
> +ENVIRONMENT_PARAMETER.asked = True
> +ENVIRONMENT_PARAMETER.store = False
> +
> +# Steps parameter. Default to a local shell script that the user defines.
> +# We do not ask this parameter, and we do not store it either.
> +STEPS_PARAMETER = ListParameter("steps", depends=NAME_PARAMETER)
> +STEPS_PARAMETER.add(DEFAULT_TESTDEF_STEP)
> +STEPS_PARAMETER.asked = True
> +STEPS_PARAMETER.store = False
> +
> +TESTDEF_TEMPLATE = {
> +    "metadata": {
> +        "name": NAME_PARAMETER,
> +        "format": DEFAULT_TESTDEF_FORMAT,
> +        "version": DEFAULT_TESTDEF_VERSION,
> +        "description": DESCRIPTION_PARAMETER,
> +        "environment": ENVIRONMENT_PARAMETER,
> +    },
> +    "run": {
> +        "steps": STEPS_PARAMETER,
> +    },
> +    "parse": {
> +        "pattern": "^\\s*(?P<test_case_id>\\w+)=(?P<result>\\w+)\\s*$"

use single quotes to avoid escaping hell?

maybe you also want to include some examples of patterns commented-out ...
(yay for YAML actually supporting comments)

> +    }
> +}
>