Merge lp://qastaging/~dooferlad/linaro-image-tools/fetch_image_cli into lp://qastaging/linaro-image-tools/11.11

 some comments from the other reviews apply ;-)

On Wed, Jun 15, 2011, James Tunnicliffe wrote:
> +import linaro_image_tools.FetchImage as FetchImage
> +import logging
> +
> +if __name__ == '__main__':

 From there, I would only call into a
    def main():
 function as to help testing and code reuse

> + file_handler = FetchImage.FileHandler()
> + config = FetchImage.Config()
> +
> + # Unfortunately we need to do a bit of a hack here and look for some options before performing
> + # a full options parse.
> + clean_cache = ( "--clean-cache" in sys.argv[1:]
> + or "-x" in sys.argv[1:])
> +
> + force_download = ( "--force-download" in sys.argv[1:]
> + or "-d" in sys.argv[1:])
> +
> + # If the settings file and server index need updating, grab them
> + file_handler.update_files_from_server(force_download)
> +
> + # Load settings YAML, which defines the parameters we ask for and acceptable responses from the user
> + config.read_config(file_handler.settings_file)
> +
> + # Using the settings that the YAML defines as what we need for a build, generate a command line parser
> + # and parse the command line
> + config.parse_args(sys.argv[1:])
> +
> + if config.args['platform'] == "snapshot":
> + config.args['release_or_snapshot'] = "snapshot"
> + else:
> + config.args['release_or_snapshot'] = "release"

 Instead of release_or_snapshot, "archive"? or "source"?

> + # Using the config we have, look up URLs to download data from in the server index
> + db = FetchImage.DB(file_handler.index_file)
> +
> + image_url, hwpack_url = db.get_image_and_hwpack_urls(config.args)
> +
> + if(image_url and hwpack_url):
> + file_handler.create_media(image_url, hwpack_url, config.args)
> + else:
> + logging.error("Unable to find files that match the parameters specified")
>

--
Loïc Minier

On 15 June 2011 18:22, Loïc Minier <email address hidden> wrote:
>  some comments from the other reviews apply   ;-)
>
> On Wed, Jun 15, 2011, James Tunnicliffe wrote:
>> +import linaro_image_tools.FetchImage as FetchImage
>> +import logging
>> +
>> +if __name__ == '__main__':
>
>  From there, I would only call into a
>    def main():
>  function as to help testing and code reuse

Noted. Will do.
<snip>
>> +    if config.args['platform'] == "snapshot":
>> +        config.args['release_or_snapshot'] = "snapshot"
>> +    else:
>> +        config.args['release_or_snapshot'] = "release"
>
>  Instead of release_or_snapshot, "archive"?  or "source"?

Release points to official Linaro releases, as currently hosted on
releases.linaro.org and shapshot the daily builds on
snapshots.linaro.org.

--
James Tunnicliffe

On Thu, Jun 16, 2011, James Tunnicliffe wrote:
> >> +    if config.args['platform'] == "snapshot":
> >> +        config.args['release_or_snapshot'] = "snapshot"
> >> +    else:
> >> +        config.args['release_or_snapshot'] = "release"
> >
> >  Instead of release_or_snapshot, "archive"?  or "source"?
>
> Release points to official Linaro releases, as currently hosted on
> releases.linaro.org and shapshot the daily builds on
> snapshots.linaro.org.

 Yup; I understood the reference, but in a way it seemed to hardcode
 that there would only be these two sources and their names in the
 variable name. It seemed like we could possibly change this over time,
 perhaps dropping one, or adding one like a ci.linaro.org download
 archive and using "release_or_snapshot" seemed a bit heavily biased
 towards exactly 2 sources of downloads.

--
Loïc Minier

On 16 June 2011 14:31, Loïc Minier <email address hidden> wrote:
> On Thu, Jun 16, 2011, James Tunnicliffe wrote:
>> >> +    if config.args['platform'] == "snapshot":
>> >> +        config.args['release_or_snapshot'] = "snapshot"
>> >> +    else:
>> >> +        config.args['release_or_snapshot'] = "release"
>> >
>> >  Instead of release_or_snapshot, "archive"?  or "source"?
>>
>> Release points to official Linaro releases, as currently hosted on
>> releases.linaro.org and shapshot the daily builds on
>> snapshots.linaro.org.
>
>  Yup; I understood the reference, but in a way it seemed to hardcode
>  that there would only be these two sources and their names in the
>  variable name.  It seemed like we could possibly change this over time,
>  perhaps dropping one, or adding one like a ci.linaro.org download
>  archive and using "release_or_snapshot" seemed a bit heavily biased
>  towards exactly 2 sources of downloads.

Ah, I see what you mean. Currently it is an indicator of the route
that the user took through the UI, which needs to be emulated for the
CLI. It is a bit hacky and I have ended up using it to build a table
name sometimes for database lookups, which I am sure I shouldn't! I
will see if I can restrict it to just the UI code where its meaning
should be more clear and think about a better name and usage.

--
James Tunnicliffe

Just a heads up that I hove committed a change in response to your feedback. I hope it covers everything.