Code review comment for lp:~terceiro/lava-dispatcher/master-device-version

Andy Doan <email address hidden> writes:

> On 12/06/2012 08:21 AM, Antonio Terceiro wrote:
>> === modified file 'lava_dispatcher/job.py'
>> --- lava_dispatcher/job.py 2012-11-24 20:58:07 +0000
>> +++ lava_dispatcher/job.py 2012-12-06 14:20:27 +0000
>> @@ -148,7 +148,6 @@
>>
>> metadata = {
>> 'target.hostname': self.target,
>> - 'target.device_version': self.context.get_device_version(),
>> }
>>
>> if 'device_type' in self.job_data:
>> @@ -232,6 +231,7 @@
>> self.context.test_data.job_status = 'fail'
>> raise
>> finally:
>> + self.context.test_data.add_metadata({ 'target.device_version': self.context.get_device_version() or 'error'})
>
> Is the get_device_version sufficiently exception-safe? ie - the 3 lines
> of code after this are important or the lava-results won't get submitted
> properly. I'm not the best at proper error handling, maybe Michael can
> speculate.

It looks ok to me, but the part that concerns me is the point at which
it is called -- get_device_version will only work usefully if the board
is in the master image, but I don't think its guaranteed to be at this
point. I would rather have the target squirrel the device version away
the first time the device boots into the master image, if that's
possible.

CHeers,
mwh

> Also - this wraps way past column 80 and is going to make
> pylint/pflakes/etc complain.
>
>> if submit_results:
>> params = submit_results.get('parameters', {})
>> action = lava_commands[submit_results['command']](
>
>
> --
> https://code.launchpad.net/~terceiro/lava-dispatcher/master-device-version/+merge/138463
> You are subscribed to branch lp:lava-dispatcher.