Another great job here, thanks! I have some comments below.

 review needs-fixing

On Thu, Jul 18, 2013 at 04:04:26PM -0000, Milo Casagrande wrote:
> Milo Casagrande has proposed merging lp:~milo/lava-tool/lava-169 into lp:lava-tool with lp:~milo/lava-tool/lava-165 as a prerequisite.
> 
> Requested reviews:
>   Antonio Terceiro (terceiro)
>   Linaro Validation Team (linaro-validation)
> 
> For more details, see:
> https://code.launchpad.net/~milo/lava-tool/lava-169/+merge/175622
> 
> This is the final step in completing card LAVA-15.
> Here there is the "status" command that polls a LAVA server and prints information about the provided job id.
> 
> The command can be invoked as:
> 
> lava status [ID]
> lava job status [ID]
> 
> The latter is the actual command and the former just calls it.

I'm not sure about the first form. `lava status` might be too generic for
people to memorize that it should be use to get status of jobs. I think we will
be able to do a better UI review after we get all the code merged in though.

> If ID is not provided, it will read the stored job IDs from the .lavaconfig file, and ask the user which job to choose.

(what follows is an updated diff from a few hours ago, not the original diff
from when the MP was created)

> === modified file 'entry_points.ini'
> --- entry_points.ini	2013-07-24 14:41:29 +0000
> +++ entry_points.ini	2013-07-24 14:41:29 +0000
> @@ -13,6 +13,7 @@
>  init = lava.commands:init
>  submit = lava.commands:submit
>  run = lava.commands:run
> +status = lava.commands:status

you can just reference lava.job.commands:status here, you don't need to write a
separate command.

BTW this also applies to a similar comment I did in the previous MP, about
cascading commands: if the arguments are the same, you can just point to the
original command from another scope in entry_points.ini and you don't have to
wrap one command inside another

>  update = lava.commands:update
>  
>  [lava_tool.commands]
> @@ -77,6 +78,7 @@
>  [lava.job.commands]
>  new = lava.job.commands:new
>  submit = lava.job.commands:submit
> +status = lava.job.commands:status
>  run = lava.job.commands:run
>  
>  [lava.device.commands]
> 
> === modified file 'lava/commands.py'
> --- lava/commands.py	2013-07-24 14:41:29 +0000
> +++ lava/commands.py	2013-07-24 14:41:29 +0000
> @@ -251,6 +251,29 @@
>          submit_cmd.invoke()
>  
>  
> +class status(BaseCommand):
> +
> +    """Checks the status of a job."""
> +
> +    @classmethod
> +    def register_arguments(cls, parser):
> +        super(status, cls).register_arguments(parser)
> +        parser.add_argument("JOB_ID",
> +                            help=("Prints a job status information."),
> +                            nargs="?",
> +                            default="-1")
> +
> +    def invoke(self):
> +        """Runs the job.status command."""
> +        from lava.job.commands import status as job_status
> +
> +        args = copy.copy(self.args)
> +        args.JOB_ID = self.args.JOB_ID
> +
> +        status_cmd = job_status(self.parser, args)
> +        status_cmd.invoke()
> +
> +
>  class update(BaseCommand):
>      """Updates a job file with the correct data."""

ditto - this should not be needed.

> === modified file 'lava/helper/command.py'
> --- lava/helper/command.py	2013-07-24 14:41:29 +0000
> +++ lava/helper/command.py	2013-07-24 14:41:29 +0000
> @@ -22,12 +22,25 @@
>  import subprocess
>  import sys
>  import types
> +import urlparse
>  
>  from lava.config import InteractiveConfig
> +from lava.parameter import Parameter
>  from lava.tool.command import Command
>  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"

these constants are used in very few places so I think it's better to just use
the literals directly.

>  class BaseCommand(Command):
>  
> @@ -41,10 +54,53 @@
>      @classmethod
>      def register_arguments(cls, parser):
>          super(BaseCommand, cls).register_arguments(parser)
> -        parser.add_argument("--non-interactive",
> +        parser.add_argument("--non-interactive", "-n",
>                              action='store_false',
>                              help=("Do not ask for input parameters."))
>  
> +    def authenticated_server(self):
> +        """Returns a connection to a LAVA server.
> +
> +        It will ask the user the necessary parameters to establish the
> +        connection.
> +        """
> +        server_name_parameter = Parameter(SERVER)
> +        rpc_endpoint_parameter = Parameter(RPC_ENDPOINT,
> +                                           depends=server_name_parameter)
> +
> +        server_url = self.config.get(server_name_parameter)
> +        endpoint = self.config.get(rpc_endpoint_parameter)
> +
> +        rpc_url = self.verify_and_create_url(server_url, endpoint)
> +        server = AuthenticatingServerProxy(rpc_url,
> +                                           auth_backend=KeyringAuthBackend())
> +        return server
> +
> +    @classmethod
> +    def verify_and_create_url(cls, server, endpoint=""):
> +        """Checks that the provided values make a correct URL.
> +
> +        If the server address does not contain a scheme, by default it will use
> +        HTTPS.
> +        The endpoint is then added at the URL.
> +        """
> +        scheme, netloc, path, params, query, fragment = \
> +            urlparse.urlparse(server)
> +        if not scheme:
> +            scheme = "https"
> +        if not netloc:
> +            netloc, path = path, ""
> +
> +        if endpoint:
> +            if not netloc[-1:] == "/" and not endpoint[0] == "/":
> +                netloc += "/"
> +            if not endpoint[-1:] == "/":
> +                endpoint += "/"
> +            netloc += endpoint
> +
> +        return urlparse.urlunparse(
> +            (scheme, netloc, path, params, query, fragment))
> +
>      @classmethod
>      def can_edit_file(cls, conf_file):
>          """Checks if a file can be opend in write mode.
> 
> === modified file 'lava/job/commands.py'
> --- lava/job/commands.py	2013-07-24 14:41:29 +0000
> +++ lava/job/commands.py	2013-07-24 14:41:29 +0000
> @@ -39,7 +39,6 @@
>  )
>  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
>  
>  
> @@ -79,8 +78,8 @@
>  
>          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 = get_key(job_instance.data,
> +                                      TESTDEF_REPOS_TAR_REPO)
>              testdef_tar_repo.set(tests_dir)
>              testdef_tar_repo.asked = True
>  
> @@ -103,14 +102,8 @@
>          jobfile = self.args.FILE
>          jobdata = open(jobfile, 'rb').read()
>  
> -        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 = self.authenticated_server()
>  
> -        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)
> @@ -119,15 +112,15 @@
>              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
> +                job_ids_parameter = ListParameter(JOBS_ID)
> +                job_ids_parameter.asked = True
>  
> -                value = self.config.get(job_id_parameter)
> +                value = self.config.get_from_backend(job_ids_parameter)
>                  if value:
> -                    job_id_parameter.set(value)
> +                    job_ids_parameter.set(value)
>  
> -                job_id_parameter.add(job_id)
> -                self.config.put_parameter(job_id_parameter)
> +                job_ids_parameter.add(job_id)
> +                self.config.put_parameter(job_ids_parameter)
>          except xmlrpclib.Fault, exc:
>              raise CommandError(str(exc))
>  
> @@ -159,3 +152,58 @@
>          else:
>              raise CommandError("The file '{0}' does not exists, or it is not "
>                                 "a file.".format(self.args.FILE))
> +
> +
> +class status(BaseCommand):
> +
> +    """Retrieves the status of a job."""
> +
> +    @classmethod
> +    def register_arguments(cls, parser):
> +        super(status, cls).register_arguments(parser)
> +        parser.add_argument("JOB_ID",
> +                            help=("Prints status information about the "
> +                                  "provided job id."),
> +                            nargs="?",
> +                            default="-1")

I would use default=None instead, since that -1 is not even a proper number.

> +
> +    def invoke(self):
> +        job_id = str(self.args.JOB_ID)
> +        # Get also the value from the config.
> +        job_ids_parameter = ListParameter(JOBS_ID)
> +        job_ids_parameter.asked = True
> +
> +        job_ids = self.config.get_from_backend(job_ids_parameter)
> +        if job_ids:
> +            job_ids = Parameter.deserialize(job_ids)
> +
> +        if job_id == "-1":

ditto

> +            # In this case we ask the user which ID to look up.
> +            if job_ids:
> +                job_id = SingleChoiceParameter("job_id",
> +                                               job_ids).prompt("Job ids: ")
> +            else:
> +                raise CommandError("No job ids stored. Please provide one "
> +                                   "on the command line.")
> +
> +        server = self.authenticated_server()
> +
> +        try:
> +            job_status = server.scheduler.job_status(job_id)
> +
> +            status = job_status["job_status"].lower()
> +            bundle = job_status["bundle_sha1"]
> +
> +            print >> sys.stdout, "\nJob id: {0}".format(job_id)
> +            print >> sys.stdout, ("Status: {0}".format(status))
> +            print >> sys.stdout, ("Bundle: {0}".format(bundle))

instead of printing the Bundle SHA1 hash, it would be more useful to provide a
URL to that bundle in the dashboard. Even more useful would be to present the
results of the tests ...  I don't know exactly yet how to do any of those,
though - presenting the URL should be a lot easier in the short term.

> +
> +            # If a job has finished running, remove it from the list of
> +            # job ids.
> +            if job_ids and status != "running" and job_id in job_ids:
> +                job_ids.remove(job_id)
> +                job_ids_parameter.set(job_ids)
> +                self.config.put_parameter(job_ids_parameter)
> +
> +        except xmlrpclib.Fault, exc:
> +            raise CommandError(str(exc))
> 

maybe we could provide a way of listing the latest N jobs submitted by the
user, together with their results. This would probably require some work on the
server API side.