Code review comment for lp:~deeptik/linaro-fetch-image/release-matrix

Hi,

Thanks for the patch. There are a couple of minor problems and I have one larger suggestion, but nothing that will take long to fix. First, this section with dates is unneeded:

+ # We only need to look up a single snapshot date. We just use the
+ # latest in the database (we could use today and search from it, but
+ # the database is just one that is checked in, so it could be old
+ # and db.get_next_prev_day_with_builds may give up before finding it).
+ date = db.execute_return_list(
+ "SELECT MAX(date) FROM snapshot_binaries")[0][0]
+ d = re.search("(\d{4})(\d{2})(\d{2})", date)
+ date = (d.group(1) + "-" + d.group(2) + "-" + d.group(3))

The date variable was only used for snapshot lookup, not for release lookup.

+ # - Run the function under test! -

This comment is now wrong

+ image_url, hwpack_url = (
+ db.get_image_and_hwpack_urls(
+ self.settings))
+ temp_list.append(self.settings['platform'])
+ img_list[self.settings['image']].append(self.settings['platform'])
+ release_list[self.settings['hardware']] = img_list

Some of these lines are >80 characters wide. Since the function has a lot of indentation I suggest you move this section into its own function to make life easier:

+ # -- Check build which matches these parameters
+ #(builds that don't match are excluded in UI)--
+ if( len(db.execute_return_list(
+ 'select * from release_hwpacks '
+ 'where platform == ? '
+ 'and hardware == ? '
+ 'and build == ?',
+ (self.settings['platform'],
+ self.settings['hwpack'],
+ self.settings['build'])))
+ and len(db.execute_return_list(
+ 'select * from release_binaries '
+ 'where platform == ? '
+ 'and image == ? '
+ 'and build == ?',
+ (self.settings['platform'],
+ self.settings['image'],
+ self.settings['build'])))):
+
+ # - Run the function under test! -
+ image_url, hwpack_url = (
+ db.get_image_and_hwpack_urls(
+ self.settings))
+ temp_list.append(self.settings['platform'])
+ img_list[self.settings['image']].append(self.settings['platform'])
+ release_list[self.settings['hardware']] = img_list

You can also add this to the end of that new function:
print "linaro-fetch-image",\
       "--hardware", self.settings['hardware'],\
       "--platform", self.settings['platform'],\
       "--image", self.settings['image'],\
       "--build", self.settings['build'],\
       "--hwpack", self.settings['hwpack']

That will print out a linaro-fetch-image command for each set of parameters we specify and we can just add on extra parameters. If we did this we could get rid of the pretty print we wouldn't have to interpret the output as much.

I did just test that print (in get_release_list) and I see that get_release_list runs even if you don't specify --list-matrix. Please only run get_release_list if needed.

+ if config.args['platform'] == 'all':
+ for platform in platform_list:
+ config.args['platform'] = platform
+ image_url, hwpack_url = db.get_image_and_hwpack_urls(config.args)
+ if(image_url and hwpack_url):
+ if config.args['exit']:
+ exit(0)
+ tools_dir = os.path.dirname(__file__)
+ if tools_dir == '':
+ tools_dir = None
+ file_handler.create_media(image_url, hwpack_url,
+ config.args, tools_dir)
+ else:
+ logging.error("Unable to find files that match the '%s' " \
+ "release", platform)

This seems to run even if a standard linaro-fetch-image command is run. Obviously we should run in batch mode or not :-)

So, on with my larger suggestion. I think we should remove the batch command (platform == all) for now. If the plan is to create a list of commands, then split each command into its own job, then lets concentrate on that. For our first CI job we can just use the first command and ignore the rest. Later we can do more.

We can modify get_release_list to produce a list restricted by the settings we give to linaro-fetch-image. For example, we can modify the outer loop to be like this:

if self.settings['hardware']
    hardware = [if self.settings['hardware']]
else:
    hardware = self.settings['choice']['hardware'].keys()

for self.settings['hardware'] in hardware:
    ...

This way, if we specify --hardware panda, it will restrict the search. We can repeat the same logic for each inner loop.

I am happy to pair program this with you if it doesn't make sense!

Thanks,

James