Code review comment for lp:~stylesen/lava-dispatcher/evolve-bundle-stream-1.6

Hello Senthil,

Thanks for your work on this. In general it looks good. :-)

I have some comments below that I think you should address before we can
merge this in, though. Nothing super complicated, but things that in my
opinion will make the code more clear for whoever comes to look at in in
future.

 review needs-fixing

> === modified file 'lava_dispatcher/actions/launch_control.py'
> --- lava_dispatcher/actions/launch_control.py 2013-01-23 22:37:05 +0000
> +++ lava_dispatcher/actions/launch_control.py 2013-03-26 13:53:02 +0000
> @@ -209,7 +209,7 @@
> if not all_bundles:
> main_bundle = {
> "test_runs": [],
> - "format": "Dashboard Bundle Format 1.5"
> + "format": "Dashboard Bundle Format 1.6"
> }
> else:
> main_bundle = all_bundles.pop(0)
>
> === modified file 'lava_dispatcher/actions/lava_test_shell.py'
> --- lava_dispatcher/actions/lava_test_shell.py 2013-02-11 02:31:22 +0000
> +++ lava_dispatcher/actions/lava_test_shell.py 2013-03-26 13:53:02 +0000
> @@ -202,18 +202,23 @@
> logging.error('Unable to get test definition from bzr\n' + str(e))
>
>
> -def _get_testdef_info(testdef):
> - metadata = {'os': '', 'devices': '', 'environment': ''}
> - metadata['version'] = str(testdef['metadata'].get('version'))
> - metadata['description'] = str(testdef['metadata'].get('description'))
> - metadata['format'] = str(testdef['metadata'].get('format'))
> +def _get_testdef_info(testdef, location):

IMO you should handle the difference between plain URL's and
repositories outside of this function. See below.

> + default = "None"

Don't you mean None (without the quotes) here? Besides, I think the rest
of the function would be more clear if you just used `None` (actual
None, not a string "None") instead of default so someone reading the
code doesn't need to wonder "what does default mean"?

> + metadata = {'os': default, 'devices': default, 'environment': default}
> + metadata['description'] = str(testdef['metadata'].get('description',
> + default))
> + metadata['format'] = str(testdef['metadata'].get('format', default))

Do you really mean to convert whatever comes from there to a string? If
the JSON contains a null (the default according to the schema MP), you
will get a string containing the word "None".

You probably want something like this:

value = testdef['metadata'].get('key'), default
metadata['key'] = value and str(value) or value

> +
> + if location == 'URL':
> + metadata['version'] = str(testdef['metadata'].get('version', default))

if you don't do anything for all other possible values of `location`, I
don't think this should be in this function, and you don't need to add
`location` as a parameter.

> # Convert list to comma separated string.
> if testdef['metadata'].get('os'):
> - metadata['os'] = ','.join(testdef['metadata'].get('os'))
> + metadata['os'] = ','.join(testdef['metadata'].get('os', default))
>
> if testdef['metadata'].get('devices'):
> - metadata['devices'] = ','.join(testdef['metadata'].get('devices'))
> + metadata['devices'] = ','.join(testdef['metadata'].get('devices',
> + default))
>
> if testdef['metadata'].get('environment'):
> metadata['environment'] = ','.join(
> @@ -249,7 +254,7 @@
> idx = len(self.testdefs)
>
> testdef_metadata = {'url': url, 'location': 'URL'}
> - testdef_metadata.update(_get_testdef_info(testdef))
> + testdef_metadata.update(_get_testdef_info(testdef, 'URL'))

See above, I don't think you should handle that logic inside
_get_testdef_info, so you should't need to add this second argument. IMO
doing the proper handling just after this snippet is fine.

> self._append_testdef(URLTestDefinition(self.context, idx, testdef,
> testdef_metadata))
>
> @@ -404,7 +409,7 @@
> f.write(self.uuid)
>
> with open('%s/testdef_metadata' % hostdir, 'w') as f:
> - f.write(yaml.dump(self.testdef_metadata))
> + f.write(yaml.safe_dump(self.testdef_metadata))
>
> if 'install' in self.testdef:
> self._create_repos(hostdir)
> @@ -439,8 +444,8 @@
> testdef_metadata = {}
> testdef_metadata.update({'url': info['branch_url']})
> testdef_metadata.update({'location': info['branch_vcs'].upper()})
> - testdef_metadata.update({'repo_rev': info['branch_revision']})
> - testdef_metadata.update(_get_testdef_info(testdef))
> + testdef_metadata.update({'version': info['branch_revision']})

What happens if the testdef in the YAML file specifies a "version"
attribute explicitly, is that just going to be overwritten?

> + testdef_metadata.update(_get_testdef_info(testdef, 'REPO'))
>
> URLTestDefinition.__init__(self, context, idx, testdef,
> testdef_metadata)
>
> === modified file 'lava_dispatcher/lava_test_shell.py'
> --- lava_dispatcher/lava_test_shell.py 2013-02-11 02:31:22 +0000
> +++ lava_dispatcher/lava_test_shell.py 2013-03-26 13:53:02 +0000
> @@ -284,6 +284,26 @@
> return attachments
>
>
> +def _get_run_testdef_metadata(test_run_dir):
> + default = "None"
> + testdef_metadata = {
> + 'version': default,
> + 'description': default,
> + 'format': default,
> + 'location': default,
> + 'url': default,
> + 'os': default,
> + 'devices': default,
> + 'environment': default
> + }

Same thing: don't you mean the actual None object instead of a
string "None"?

And again, I think it would be more clear if you just used None directly
instead of that variable