Code review comment for lp:~milo/lava-tool/lava-169

Hi Antonio,

I updated the code.
I also pushed here the "script" commands. Please, take a look at them.

The big part of the refactoring happened on the lava-165 merge.

On Wed, Jul 24, 2013 at 10:46 PM, Antonio Terceiro
<email address hidden> wrote:
>
> 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.

Indeed. Also, it would be better to do it with someone that will
actually use the tool.

>> === 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

There is actually only the status one, the run and submit are all a
little bit different, in particular when you start from a testdef or a
script file.

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

Cleared them out.

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

Fixed.

> 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.

> 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.

I actually removed this from both lava-165 and lava-169.
I want to address that with another merge proposal, but after
everything has been merged.

Will file a bug to keep track of it, and fix the config/cache
"problem" via pyxdg once this code is landed.

Also for the "bundle" one: I prefer to not fix it now, but keep track
of it with a bug. It is tied to the config/cache "problem" since we
need to store information of that job to make the URL (we need to keep
track of the server URL, the stream, and those might change even from
job to job).

--
Milo Casagrande | Automation Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs