[Avocado-devel] Implement fetch-assets command line

[Amador Pahim]

On Thu, Sep 26, 2019 at 5:34 PM Cleber Rosa <crosa at redhat.com> wrote:
>
> On Thu, Sep 26, 2019 at 10:08:47AM +0200, Amador Pahim wrote:
> > On Thu, Sep 26, 2019 at 9:49 AM Lukáš Doktor <ldoktor at redhat.com> wrote:
> > >
> > > Hello guys,
> > >
> > > what a nice discussion. But before you start, what lead you to pick this card? It's one of the nice-to-have-ideas card and we have plenty of well-defined-and-useful cards that also needs attention, so unless you have a real-world usage, I'd probably suggest to focus on something you can directly benefit from. (unless you have other interest in eg. learning something, or other kind of interest).
> >
> > Fair point. A RFC would do some good here.
> >
>
> Expanding the description on the card and linking the QEMU threads
> would be a good idea too.
>
> > >
> > > Dne 25. 09. 19 v 23:39 Cleber Rosa napsal(a):
> > > > On Wed, Sep 25, 2019 at 06:01:22PM -0300, Willian Rampazzo wrote:
> > > >> After going thru the suggestions, and thinking about it, I came up
> > > >> with the following starting point:
> > > >>
> > > >> 1 - Support a limited test source code parsing. This feature brings a
> > > >> simple solution to those tests already using strings into their
> > > >> fetch_asset parameters, as stated by Cleber.
> > > >>
> > > >
> > > > Keep in mind that I'm assuming this is a viable option without
> > > > attempting to do it myself.  I hope it's viable, because it makes
> > > > "simple things easy" from the test writer's side.
> > > >
> > >
> > > Yes, this makes sense to me and would be useful even for some human-asset interface (list assets, list cached assets, see details of cached assets...). Btw cache-management interface is IMO higher in the priority than actually fetching the assets without executing tests (but it's just my priority list, don't take this as an official statement).
> > >
> > > >> 2 - Add an option to use assets.json, as stated by Amador. This file
> > > >> would sit in the test data folder and list all assets that are
> > > >> necessary for the test. As a list of assets, I thought about a new
> > > >> method, fetch_assets, as a wrapper that load all assets from the file
> > > >> and call fetch_asset for each one.
> > > >>
> > > >
> > > > Sounds good.  Also consider the need for dynamic asset definition (the
> > > > idea about executable scripts that produces JSON).

So, if you are going for the implementation of the two methods, they
should probably be related. Either by having the code parser
generating the configuration file that will be then used by the cacher
implemented in the option 2 or by having the code parser as the first
of the dynamic plugins mentioned by Cleber. Just thinking out loud
here.

> > > >
> > >
> > > Hmm, this is tricky. I don't like much having "reserved" file names as people might get unwanted clashes. Yes, it should be fairly harmless with a json file (either it matches or is skipped) but it's a semi-hidden feature. We also have the https://trello.com/c/WPd4FrIy/1479-add-support-to-specify-assets-in-test-docstring that sounds more explicit to me. Another alternative I'd prefer is a `Test` class variable, which can be obtained by a static analysis, or even `Test` variable initialized on `__init__`, which could be executed by the asset fetcher interface (when asked for, because it might eventually execute some nasty code). Having a pre-defined file names in datadir locations is the last interface I'd like to see in Avocado (out of these).
> > >
> >
> > I don't think reserved filenames are a good way either. The baseline
> > from my suggestion is an external file containing the arguments that
> > will be used by both the asset.Asset(from_file=filename.json) and by
> > the command line tool that will cache things: avocado fetch-asset
> > filename.json filename2.json filename3.json.
> >
> >
>
> I disagree, because I am big a believer in well known conventions.
>
> Just for the sake of comparison, we can't have "#!" as the first chars
> in an executable file and not have the following characters treated as
> the interpreter because of a convention.  To miminize clashes, we need
> to pick good names, for instance, I find it hard that someone will
> *have to* name a file "avocado_assets.json" inside a directory named
> "$MY_TEST.data" for any other reason.

Thinking about it, that would actually make easier to connect the two
implementations, case the code parser is generating those config
files. But that's too far in the discussion already and we should
probably save it for the pull requests.

Willian, thank you for raising this topic and for tackling this
feature. Looking forward to having it.

>
> Finally, if that's still a valid scenario, a configuration for the
> naming can be defined and activated for one particular installation,
> user or execution.  The number of "normal" occurrences though, and the
> saved time and convenience of those conventions, IMO, prove
> themselves.
>
> > > As before, if you have some real-world use case, please share it as it can change everything.
> > >
> > >
> > > class variable
> > > --------------
> > >
> > >     class MyTest(avocado.test):
> > >         assets = {"foo": ["http://aaa"], "bar": {"location": "http://fdsa", hash="..."}}
> >
> > This seems like a good alternative, but it also seems like one more
> > source or parametrization. I'd then use the parametrization interface
> > instead.
> >
> > >
> > > initialized variable
> > > --------------------
> > >
> > >     class MyTest(avocado.test):
> > >         def __init__(self, ...):
> > >             self.assets = {}
> > >             for asset in something:
> > >                  self.assets[asset] = get_asset_url(asset)
> >
> > I would not ask users to deal with __init__ in their test classes.
> >
>
> I agree, the Avocado.Test interface for __init__ is ugly and users
> should as much as possible be kept away from it.
>
> - Cleber.
>
> > >
> > > ---
> > >
> > > Regards,
> > > Lukáš
> > >
> >
> >
> > --
> > Pahim
>


-- 
Pahim