On Wed, Jul 24, 2013 at 2:49 AM, Antonio Terceiro
<email address hidden> wrote:
>> +
>> +INIT_TEMPLATE = {
>> +    JOBFILE_ID: JOBFILE_PARAMETER,
>> +}
>
> Do we really need a dictionary with a single key?

If we want to maintain the same pattern as for the other commands,
yes: go through the template, look up the parameter in the config, and
ask for the values there.
Or we need to change the "expand_template" function signature and also
the name to accept single Parameter objects.

>> +    def _create_files(self, data, full_path, test_path):
>> +        # This is the default script file as defined in the testdef template.
>> +        default_script = os.path.join(test_path, DEFAULT_TESTDEF_SCRIPT)
>> +
>> +        if not os.path.isfile(default_script):
>> +            # We do not have the default testdef script. Create it, but
>> +            # remind the user to update it.
>> +            print >> sys.stdout, ("\nCreating default test script "
>> +                                  "'{0}'.".format(DEFAULT_TESTDEF_SCRIPT))
>> +
>> +            with open(default_script, "w") as write_file:
>> +                write_file.write(DEFAULT_TESTDEF_SCRIPT_CONTENT)
>
> if this behaviour of "create a file with this content here" starts to repeat,
> we probably want to encapsulatet it somewhere else (I noted it at least once in
> other parts of this MP).

Sounds good, even if at the moment should be only used here.
It was used also in the tests to fill up files with fake content.

>> +            # Prompt the user to write the script file.
>> +            self.edit_file(default_script)
>> +
>> +        # Make sure the script is executable.
>> +        os.chmod(default_script,
>> +                 stat.S_IRWXU | stat.S_IRWXG | stat.S_IROTH | stat.S_IXOTH)
>> +
>> +        print >> sys.stdout, ("\nCreating test definition "
>> +                              "'{0}':".format(DEFAULT_TESTDEF_FILE))
>
> print already writes to stdout by default. is it really necessary to be that explicit?

I used the same behavior found in most of lava-tool (in particular in
lava_dashboard_tool) where it is using the explicit redirect.
Albeit, there is a mixed use of it even in the old code.

> I’m not sure about this approach of calling other commands. IMO the best
> approach would be to concentrate as much of the logic as possible in the
> model/backend classes, them call them from both commands.

Some refactoring will be needed, not sure if in this merge proposal though.

> IIUC you are assuming that you will be always storing the tarball in the job
> file?

True only if you go through the "lava init" step.
If you want to use the other commands, this does not happen. It might
need some refactoring since "testdef|script submit" is still missing
though.

> My initial thinking was to always generate a tarball dinamically, so that
> you don’t have to explicitly update a job file. It would go like this:
>
> $ lava job submit FOO.json
>
>   submits FOO.json as ir

This works.

> $ lava testdef submit FOO.yaml
>
>   creates the tarball and a job file dinamically; submit those
>
> $ lava script submit FOO.sh
>
>   creates testdef, tarball and job file dynamically; submit them

These two are still missing.

After showing this at Connect though, the common patterns were:
- I will not use this (mostly from QA or people that knows how to write testdef)
- There are too many commands (from a couple of Linaro employees and
ARM ones too, hence the "lava init" stuff that will do everything
almost automatically).

> what is the difference between using this Singleton implementation and just
> instantiating a single object during initialization and exposing that object to
> be used by other modules?

The Config object is instantiated in the BaseCommand class, class that
all the other commands inherits. If I turn that into a class
attribute, or even a module one:

class BaseCommand():

 CONFIG = InteractiveConfig( ... )
 __init__():
  self.config = self.CONFIG

I need to change how the interactiviness of the Config class is set,
since I cannot control it directly anymore. I can add a
"set_force_interactive" method for that and call it from the
BaseCommand class.
It was mostly used when calling other commands from another one, since
I needed the same Config object shared between them. Changing that
behavior, this will hopefully not be needed anymore.

>> +        # Mostly needed for testing purposes.
>> +        if config_file is not None:
>> +            self._config_file = config_file
>> +        else:
>> +            self._config_file = (os.environ.get('LAVACONFIG') or
>> +                                 os.path.join(os.path.expanduser('~'),
>> +                                              '.lavaconfig'))
>
> you could instead turn config_file into a property with lazy initialization,
> then for testing you could mock that method to make it return the config data
> you want.

Sounds good.

>>      @classmethod
>> +    def retrieve_file(cls, path, extensions):
>> +        """Searches for a job file in the provided path.
>> +
>> +        :return The job file full path.
>> +        """
>
> it look to me that this method could be generic. If that doesn’t make sense, it
> could probably be called `find_job_file` or something like that.

It is generic, it's the doc string that is misleading.

>> +    @classmethod
>> +    def verify_file_extension(cls, path, default, supported):
>> +        # Checks that the file we pass has an extension or a correct one.
>> +        # If not, it adds the provided one.
>
> this comment is rather confusing

Fixed now, turned it into a doc string also.

>> +
>> +    def tmp(self, name):
>> +        """Returns a path to a non existent file/directory.
>
> A method called `tmp` that returns a non existing file is rather confusing. Can
> we come up with a better name? Maybe the method name is correct but the
> docstring needs to say something less confusing.
>
> What about
>
> """
> Returns the full path to a file called `name` in a temporary directory. This
> method does not create the file, it only gives a full filename where you can
> actually write some data.
> """

+1

> I don't think we should keep the list of submitted jobs in the config file.
> Think of someone that wants to keep their lava config file under version
> control (I do that for most of all my config files); that user won't want
> lava-too to change the config file for every job submission.
>
> Instead, I think the list of job ids (and eventually some extra data about each
> job) is runtime data, and as such should be kept under ~/.cache/lava/jobs or
> something like that

Sounds good.
But I'm not fixing it with this merge proposal. I will do it in
another one, since I guess it is better if we use pyxdg for this. Not
sure if we should also move the ".lavaconfig" file into
$XDG_CONFIG_HOME.

>> +ACTIONS_ID = "actions"
>> +COMMAND_ID = "command"
>> +IMAGE_ID = "image"
>> +PARAMETERS_ID = "parameters"
>> +DEVICE_TYPE_ID = "device_type"
>> +TESTDEF_REPOS_ID = "testdef_repos"
>> +TESTDEF_REPOS_TAR_REPO = "tar-repo"
>> +STREAM_ID = "stream"
>> +SERVER_ID = "server"
>
> I don’t think this many constants are usefull. I would rather expect the actual
> template data to be as close to the actual data as possible and believe that
> will make their maintaince easier.

Will clean them out.

>> -            user_input = raw_input(prompt).strip()
>> +            data = raw_input(prompt).strip()
>>          except EOFError:
>> -            pass
>> +            # Force to return None.
>> +            data = None
>>          except KeyboardInterrupt:
>>              sys.exit(-1)
>> -
>> -        if user_input is not None:
>> +        return data
>> +
>> +    @classmethod
>> +    def serialize(cls, value):
>> +        """Serializes the passed value to be friendly written to file.
>> +
>> +        Lists are serialized as a comma separated string of values.
>
> I already saw something about serializing lists above. Can we keep all this
> list serialization/deserialization just here?

What do you mean? Having a single method to serialize and deserialize
instead of the two we have right now?
"serialize" and "deserialize" are in the same class (Parameter), the
LIST_SERIALIZE_DELIMITER is a module constant.

>> +        :param value: The value to serialize.
>> +        :return The serialized value as string.
>> +        """
>> +        serialized = ""
>> +        if isinstance(value, list):
>> +            serialized = LIST_SERIALIZE_DELIMITER.join(
>> +                str(x) for x in value if x)
>> +        else:
>> +            serialized = str(value)
>> +        return serialized
>
> I wonder if we can we use a existing serialization instead of using a arbitrary
> delimiter ...  e.g. json + base64, this way we don’t have to limit ourselves to
> values that do not contain a comma. But OTOH we want something that is readable
> (and editable!) in the config file ...

I wanted to use something that could have been human readable
(avoiding also all hashing mechanism), and easily to modify.
I considered also pickle, but then I would have preferred to pass
everything through it rather than just a small set of values.
We can probably use just json.dumps() and json.loads(), even if I'm
not so sure about that... why not using just json or YAML in the
config that have better marshaling support?

> As far as this MP is concerned this class is not used (apart from its own unit
> tests). This is the one you mentioned in the introduction right?
>
> I can't see the point of a URL scheme parameter, I think it's easier to just
> ask for the URL itself and then deduce the scheme from there.

Yes. Right now it is not needed anymore, I thought I had removed it in
the other MP, but
This was done to infer the scheme type when at the beginning we wanted
to use "data:" to send base64 encoded content.

> As I commented above, this TarRepoParameter class should not be needed (i.e.
> users should not need to deal with tarballs themselves)

Will refactor this out.

> I don't think that insuficient permissions are the only possible cause of an
> IOError here ...

Yep, fixed.

>> === added file 'lava/testdef/templates.py'
>> +# This is what will be called by default by the test definition yaml file.
>> +DEFAULT_TESTDEF_SCRIPT = "mytest.sh"
>> +DEFAULT_TESTDEF_SCRIPT_CONTENT = """#!/bin/sh
>> +# Automatic generated content by lava-tool.
>> +# Please add your own instructions.
>> +"""
>
> It would be nice to include some commented examples of the lava-test-shell API
> usage ...

Will add them, sure.

>> +DEFAULT_TESTDEF_STEP = "./mytest.sh"
>> +
>> +DEFAULT_ENVIRONMET_VALUE = "lava_test_shell"
>> +
>> +# All these parameters will not be stored on the local config file.
>> +NAME_PARAMETER = Parameter("name")
>> +NAME_PARAMETER.store = False
>> +
>> +DESCRIPTION_PARAMETER = Parameter("description", depends=NAME_PARAMETER)
>> +DESCRIPTION_PARAMETER.store = False
>> +
>> +ENVIRONMENT_PARAMETER = ListParameter("environment",
>> +                                      depends=NAME_PARAMETER)
>> +ENVIRONMENT_PARAMETER.add(DEFAULT_ENVIRONMET_VALUE)
>> +ENVIRONMENT_PARAMETER.asked = True
>> +ENVIRONMENT_PARAMETER.store = False
>> +
>> +# Steps parameter. Default to a local shell script that the user defines.
>> +# We do not ask this parameter, and we do not store it either.
>> +STEPS_PARAMETER = ListParameter("steps", depends=NAME_PARAMETER)
>> +STEPS_PARAMETER.add(DEFAULT_TESTDEF_STEP)
>> +STEPS_PARAMETER.asked = True
>> +STEPS_PARAMETER.store = False
>> +
>> +TESTDEF_TEMPLATE = {
>> +    "metadata": {
>> +        "name": NAME_PARAMETER,
>> +        "format": DEFAULT_TESTDEF_FORMAT,
>> +        "version": DEFAULT_TESTDEF_VERSION,
>> +        "description": DESCRIPTION_PARAMETER,
>> +        "environment": ENVIRONMENT_PARAMETER,
>> +    },
>> +    "run": {
>> +        "steps": STEPS_PARAMETER,
>> +    },
>> +    "parse": {
>> +        "pattern": "^\\s*(?P<test_case_id>\\w+)=(?P<result>\\w+)\\s*$"
>
> use single quotes to avoid escaping hell?
> maybe you also want to include some examples of patterns commented-out ...
> (yay for YAML actually supporting comments)

Sounds good.

-- 
Milo Casagrande | Automation Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs