Please have a look at the "Implement match_host_* in libtaskotron" section of the design document I shared with you via google-drive , and change the code accordingly. This is a good start, though.

Why not use a function that returns a dict() instead? Seems like a cannon to kill a fly. Not that there is something functionally wrong with it.

I'd rather see the playbook.get('vars', {}) part separated to something like:

    vars = playbook.get('vars', {})

Honestly this whole thing is a bit repetetive, and could be done like:

    vars = playbook.get('vars', {})
    for var in ['taskotron_match_host_distro', 'taskotron_match_host_release', ...]:
        setattr(self, var, vars.get(var, False))
        log.info("Variable %s is %s", var, getattr(self, var))

Not saying this is necessarily better, though

Please also remove the default None values from the method signature.

I'm fairly certain we could just get rid of all the log.debug calls, and just replace all of those with something like:

log.debug('Inferred data: %s', env)

Here and

log.debug('Devised environmnet: %s', env)

just before the return statement

rework lbrabec's patch

It uses dict for data management now. I used the opportunity to
restructure it a bit, all available variables are now documented in
executor.py. The variables are saved into a file to debugging, including
the new ones. There's a new method that takes care of parsing variables
and populating the dictionary, keeping the logic in one place. The
number of debug printouts was reduced where they seemed unnecessary.

A bit unrelated change is removing root=bool support to run the playbook
as a standard user. It was a non-complete feature (not hooked up to
cmdline args), never used, very probably broken. I didn't want to spend
time to hook it up properly to the updated workflow. We can add it back
later if needed.

Please note that testsuite is broken at the moment, will fix it when I
get ack on the patch.

Please move the comments to the end of line. This is really unreadable. I also do not really understand the change. Why move the "docs" from what is the location one could possibly look for it to some random source file?

IMO this also makes fair bit harder to reconfigure these. Previously, this was a 'static file' provided by the libtaskotron package. Now the changes (e.g. heartbeat_interval) need to be done by much nastier way of hacking a source code, should one want to change it.

It's long yes, but it made little sense to me to have all the vars defaults in executor.py, and then have most of them (but not all of them) duplicated in runner.yml together with their description. It would get out of sync all the time. I can move it to the end of file, if you prefer, sure.

I don't follow the "changes need to be done nastier" comment. The heartbeat_interval wasn't configurable before, and it still isn't. If we want it configurable, we can expose it in taskotrong config. I doubt it's useful, though.

Some of the "default" values are also absolutely disregarded (caught varsfile on the first sight, decided not to go check all the others).

Good catch, will fix varsfile.

move vars template to end of file, use varsfile default

@kparal - yes, the value was not in config.py, but if you wanted to change it, the default was not "hidden" inside a source code, but in a pretty manageable place in /usr/share/libtaskotron/ansible. Say we wanted to change some of the default values in our deployment. The only way would be either releasing a new version, or hot-fixing the executor.py. In the previous state, one could simply use ansible+template to change the runner.yml file

Please check also all the other variables, when you are at it.

Not sure which are the "allowed" taskotron_* values in the playbook, but it might make sense to do this after the default values are set.

Still valid

Please add the debug log which contains only the actually "devised" data before defaults are applied.

Why not move this whole thing to the top, and just set the distro/release/flavor/arch from the defaults to begin with?

I'd like to see the possible KeyError handled.

I don't really agree on anything in /usr/share being "configurable". If you need to hotpatch something, you can do it the same way with a yaml file or a python file. I don't really see a difference, it's a source code change in both cases, just a different file format. Also, the only thing that you really could do this previously was the heartbeat/keepalive values (everything else got overridden). Is it that important to be able to hotfix hearbeat default in yaml instead of python to discuss this topic?

It doesn't raise KeyError, None is default.

Is it that important to be able to hotfix hearbeat default in yaml instead of python to discuss this topic?

It is a poster child for why I don't like the change. So, yes it is important to discuss it :)

Currently you see a printout every time a value is forced ("devised" if you want to call it). In the end, anything that hasn't been forced is a default value. It seemed quite clear to me, is it not?

Also, I'm confused by your previous comment. If I move the 4 lines to the top and set it as default values right away, how do you want me to print the values before the defaults are applied?

Sorry, this comment is displayed at the top of vm.py. I have no idea which method you're referring to.

Not sure which are the "allowed" taskotron_* values in the playbook, but it might make sense to do this after the default values are set.

Does this refer to the beginning of _create_playbook_vars? I even added a comment:

        # load all provided taskotron_* vars first, so that they don't override
        # out logic later on

This needs to be done as the first thing, otherwise people could override some vars we expose to them later. See
https://qa.fedoraproject.org/docs/libtaskotron/latest/writingtasks.html#special-task-variables

The alternative approach is to list which variables we read from tests.yml and read only those, instead of taskotron_*. But that means having yet another list to maintain, and this seemed easier and pretty safe.

It is a poster child for why I don't like the change. So, yes it is important to discuss it :)

Well then tell me what you'd like to see changed, in a generic fashion. I don't understand it currently. You're not complaining about not being able to change e.g. arg_data in the middle of the execution. Why should you be able to change ansible data? And why is moving some (a few!) of the defaults into the playbook file worth the increased complexity (more logic), worse readability (definitions scattered in several places) and increased maintenance (vars going out of sync in different places)?

Please review again.

Since we both ended up dissatisfied, I think this is a worthy compromise :-D

Fine to merge, once the testsuite is updated.

Functinaly, this probably works as intended. Code looks good. Vast amounts of yacks could be shaved, but I've been unreasonable enough for a patch of this insignificance already.

THX for all the relevant changes!

Commit 1f8884bd fixes this pull-request

And this is now live on dev, and working :-)