Merge lp:~stylesen/lava-dispatcher/evolve-bundle-stream-1.6 into lp:lava-dispatcher

[Senthil Kumaran S]

Evolve bundle stream version to 1.6 due to the addition of testdef metadata.

[Zygmunt Krynicki (zyga)]

- # XXX testdef_metadata = yaml.safe_load(testdef_metadata)
28 + testdef_metadata = yaml.load(testdef_metadata)

Given the recent fuckup with using yaml I would rather see safe_load here.

-1

review: Needs Fixing

[lp:~stylesen/lava-dispatcher/evolve-bundle-stream-1.6]

554. By Senthil Kumaran S on 2013-03-25

Add "None" string as default value for testdef metadata.

555. By Senthil Kumaran S on 2013-03-25

Bring up-to-date with trunk.

556. By Senthil Kumaran S on 2013-03-25

Add default values for testdef metadata based on new bundle format.

557. By Senthil Kumaran S on 2013-03-25

Populate the version properly based on URL or REPO location.

558. By Senthil Kumaran S on 2013-03-25

Fix a typo on if condition.

559. By Senthil Kumaran S on 2013-03-25

Use safe_* for YAML processing.

[Antonio Terceiro (terceiro)]

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'] =...

review: Needs Fixing

[Senthil Kumaran S (stylesen)]

Hi Antonio,

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

I fixed all the review comments from you. I request you to go through the new changes and approve this MP.

Thank You.
--
Senthil Kumaran
http://www.stylesen.org/
http://www.sasenthilkumaran.com/

[lp:~stylesen/lava-dispatcher/evolve-bundle-stream-1.6]

560. By Senthil Kumaran S on 2013-04-02

Remove URL handling from '_get_testdef_info' and usage of "None" string
as default value.

[Antonio Terceiro (terceiro)]

Hi Senthil,

In general, I like the fact that we are getting the diff smaller and
smaller. :-)

Follows another round of comments, which is basically one issue really:
the issue of default values - I think it should be `None`, and would
like your opinion.

 review needs-info

> === 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-04-02 01:15:27 +0000
> @@ -204,20 +204,20 @@
>
> 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'))
> + metadata['description'] = testdef['metadata'].get('description', '')
> + metadata['format'] = testdef['metadata'].get('format', '')
> + metadata['version'] = testdef['metadata'].get('version', '')
>
> # 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', ''))
>
> if testdef['metadata'].get('devices'):
> - metadata['devices'] = ','.join(testdef['metadata'].get('devices'))
> + metadata['devices'] = ','.join(testdef['metadata'].get('devices', ''))
>
> if testdef['metadata'].get('environment'):
> metadata['environment'] = ','.join(
> - testdef['metadata'].get('environment'))
> + testdef['metadata'].get('environment', ''))

So you changed from using `"None"` to using an empty string as the
default. Why are we doing this? Is this the expected behavior?

If you agree with me that None is indeed the appropriate default here, I
think you should just undo the diff lines above.

> === 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-04-02 01:15:27 +0000
> @@ -284,6 +284,25 @@
> return attachments
>
>
> +def _get_run_testdef_metadata(test_run_dir):
> + testdef_metadata = {
> + 'version': '',
> + 'description': '',
> + 'format': '',
> + 'location': '',
> + 'url': '',
> + 'os': '',
> + 'devices': '',
> + 'environment': ''
> + }

minor nitpick: shouldn't this closing brace align with testdef_metadata (i.e. one
indent level to the left)?

Main point here is: why are you using an empty string as default values,
instead of `None`? Isn't `None` the appropriate value in case no data
was provided? Shouldn't we s/''/None/ in the above hunk?

--
Antonio Terceiro
Software Engineer - Linaro
http://www.linaro.org

review: Needs Information

[lp:~stylesen/lava-dispatcher/evolve-bundle-stream-1.6]

561. By Senthil Kumaran S on 2013-04-05

Make the default values as None instead of empty strings.

Suggested by: terceiro

[Senthil Kumaran S (stylesen)]

Hi Antonio,

On Thu, 2013-04-04 at 00:22 +0000, Antonio Terceiro wrote:
> > # 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', ''))
> >
> > if testdef['metadata'].get('devices'):
> > - metadata['devices'] = ','.join(testdef['metadata'].get('devices'))
> > + metadata['devices'] = ','.join(testdef['metadata'].get('devices', ''))
> >
> > if testdef['metadata'].get('environment'):
> > metadata['environment'] = ','.join(
> > - testdef['metadata'].get('environment'))
> > + testdef['metadata'].get('environment', ''))
>
> So you changed from using `"None"` to using an empty string as the
> default. Why are we doing this? Is this the expected behavior?
>
> If you agree with me that None is indeed the appropriate default here, I
> think you should just undo the diff lines above.

I agree that None is the correct default, which I fixed in the code. But
for devices, os, environment having an empty string as default while
retrieving the values from the dict is fine, since we do a join of these
strings with a comma.

> > === 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-04-02 01:15:27 +0000
> > @@ -284,6 +284,25 @@
> > return attachments
> >
> >
> > +def _get_run_testdef_metadata(test_run_dir):
> > + testdef_metadata = {
> > + 'version': '',
> > + 'description': '',
> > + 'format': '',
> > + 'location': '',
> > + 'url': '',
> > + 'os': '',
> > + 'devices': '',
> > + 'environment': ''
> > + }
>
> minor nitpick: shouldn't this closing brace align with testdef_metadata (i.e. one
> indent level to the left)?

That is what emacs python mode gives me. I tried a tab on that line, but
emacs refused to align it! Hope this the correct indentation? (I go by
emacs ;) )

I fixed the comments which you mentioned above, please go through it and let me know
if it is good to be merged.

Thank You.

--
Senthil Kumaran
http://www.stylesen.org/
http://www.sasenthilkumaran.com/

[lp:~stylesen/lava-dispatcher/evolve-bundle-stream-1.6]

562. By Senthil Kumaran S on 2013-04-05

Bring up-to-date with trunk.

[Antonio Terceiro (terceiro)]

On Fri, Apr 05, 2013 at 09:33:20AM -0000, Senthil Kumaran S wrote:
> Hi Antonio,
>
> On Thu, 2013-04-04 at 00:22 +0000, Antonio Terceiro wrote:
> > > # 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', ''))
> > >
> > > if testdef['metadata'].get('devices'):
> > > - metadata['devices'] = ','.join(testdef['metadata'].get('devices'))
> > > + metadata['devices'] = ','.join(testdef['metadata'].get('devices', ''))
> > >
> > > if testdef['metadata'].get('environment'):
> > > metadata['environment'] = ','.join(
> > > - testdef['metadata'].get('environment'))
> > > + testdef['metadata'].get('environment', ''))
> >
> > So you changed from using `"None"` to using an empty string as the
> > default. Why are we doing this? Is this the expected behavior?
> >
> > If you agree with me that None is indeed the appropriate default here, I
> > think you should just undo the diff lines above.
>
> I agree that None is the correct default, which I fixed in the code.

Cool.

I like get('foo') -- how it was before -- more then get('foo', None), though.

> But for devices, os, environment having an empty string as default
> while retrieving the values from the dict is fine, since we do a join
> of these strings with a comma.

The joins are already inside if's that test for the presence of those
properties. If present, are't those properties guaranteed to be arrays
according to the schema?

--
Antonio Terceiro
Software Engineer - Linaro
http://www.linaro.org

[lp:~stylesen/lava-dispatcher/evolve-bundle-stream-1.6]

563. By Senthil Kumaran S on 2013-04-07

Do not have empty strings while joining device, os and environment metadata.

[Senthil Kumaran S (stylesen)]

Hi Antonio,

On Sun, 2013-04-07 at 13:37 +0000, Antonio Terceiro wrote:
> > But for devices, os, environment having an empty string as default
> > while retrieving the values from the dict is fine, since we do a join
> > of these strings with a comma.
>
> The joins are already inside if's that test for the presence of those
> properties. If present, are't those properties guaranteed to be arrays
> according to the schema?

I cross checked it (and tested it in my local instance) and understood
it is not required to due to the if condition. I fixed it in r563.
Waiting for your review/approval :)

Thank You.

--
Senthil Kumaran
http://www.stylesen.org/
http://www.sasenthilkumaran.com/

[Antonio Terceiro (terceiro)]

On Sun, Apr 07, 2013 at 05:26:20PM -0000, Senthil Kumaran S wrote:
> I cross checked it (and tested it in my local instance) and understood
> it is not required to due to the if condition. I fixed it in r563.
> Waiting for your review/approval :)

Go! :-)

 review approve

--
Antonio Terceiro
Software Engineer - Linaro
http://www.linaro.org

review: Approve

[lp:~stylesen/lava-dispatcher/evolve-bundle-stream-1.6]

564. By Senthil Kumaran S on 2013-04-07

Bring up-to-date with trunk.

=== 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-04-08 01:02:21 +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-04-08 01:02:21 +0000
@@ -204,9 +204,9 @@
 
 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'))
+    metadata['description'] = testdef['metadata'].get('description', None)
+    metadata['format'] = testdef['metadata'].get('format', None)
+    metadata['version'] = testdef['metadata'].get('version', None)
 
     # Convert list to comma separated string.
     if testdef['metadata'].get('os'):
@@ -404,7 +404,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 +439,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']})
 
         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-04-08 01:02:21 +0000
@@ -284,6 +284,25 @@
     return attachments
 
 
+def _get_run_testdef_metadata(test_run_dir):
+    testdef_metadata = {
+        'version': None,
+        'description': None,
+        'format': None,
+        'location': None,
+        'url': None,
+        'os': None,
+        'devices': None,
+        'environment': None
+        }
+
+    metadata = _read_content(os.path.join(test_run_dir, 'testdef_metadata'))
+    if metadata is not '':
+        testdef_metadata = yaml.safe_load(metadata)
+    
+    return testdef_metadata
+
+
 def _get_test_run(test_run_dir, hwcontext, build, pkginfo, testdefs_by_uuid):
     now = datetime.datetime.utcnow().strftime('%Y-%m-%dT%H:%M:%SZ')
 
@@ -292,11 +311,9 @@
     uuid = _read_content(os.path.join(test_run_dir, 'analyzer_assigned_uuid'))
     attachments = _get_run_attachments(test_run_dir, testdef, stdout)
     attributes = _attributes_from_dir(os.path.join(test_run_dir, 'attributes'))
-    # XXX testdef_metadata = _read_content(os.path.join(test_run_dir,
-    # XXX                                              'testdef_metadata'))
 
     testdef = yaml.safe_load(testdef)
-    # XXX testdef_metadata = yaml.safe_load(testdef_metadata)
+
     if uuid in testdefs_by_uuid:
         sw_sources = testdefs_by_uuid[uuid]._sw_sources
     else:
@@ -314,8 +331,8 @@
         'hardware_context': hwcontext,
         'attachments': attachments,
         'attributes': attributes,
-        # XXX 'testdef_metadata': testdef_metadata,
-    }
+        'testdef_metadata': _get_run_testdef_metadata(test_run_dir)
+        }
 
 
 def _read_content(filepath, ignore_missing=False):
@@ -354,4 +371,4 @@
             except:
                 logging.exception('error processing results for: %s' % test_run_name)
 
-    return {'test_runs': testruns, 'format': 'Dashboard Bundle Format 1.5'}
+    return {'test_runs': testruns, 'format': 'Dashboard Bundle Format 1.6'}
 
=== modified file 'lava_dispatcher/test_data.py'
--- lava_dispatcher/test_data.py	2013-01-30 05:33:55 +0000
+++ lava_dispatcher/test_data.py	2013-04-08 01:02:21 +0000
@@ -34,8 +34,7 @@
     def __init__(self, test_id='lava'):
         self.job_status = 'pass'
         self.metadata = {}
-        self._test_run = { 'test_results':[], 'attachments':[], 'tags':[], }
-                           # XXX 'testdef_metadata':{} }
+        self._test_run = { 'test_results':[], 'attachments':[], 'tags':[] }
         self._test_run['test_id'] = test_id
         self._assign_date()
         self._assign_uuid()