Not that we need to solve it right now, but in the future, I'd like to decouple this (and possibly other files that change continuously) from the "core" Taskotron, maybe into a separate library, or create a way to fetch the config file from database. But I'd like to avoid the situation we had in AutoQA, where we needed to release new AutoQA with every config/test change.

Kamil suggested getting this specific file from something like http://mirrors.fedoraproject.org/releases.txt which is usually outdated, but we could (if we don't decide to go any other way) maintain it, since we are maintaining this custom file anyway.

s/XMLRPC/RESTful/

Cool, seems like a good base for T85

Not sure this is necessarily required, but just to be sure:

return isinstance(obj, collections.Iterable) and not isinstance(obj, (str, unicode))

Would you consider renaming "test.py" to e.g. "test_detail.py" - test.py is (IMHO) usually used for unittests and such.

nirik says that file was mainly for preupgrade and they're considering to remove it completely. I was suggested to look at liveusb-creator or fedup, how they query for such information. Also pkgdb2 (on it's way to production) could help us here:
http://209.132.184.188/api/collections
http://209.132.184.188/api/

Good idea, thanks.

According to our latest discussion, I'll rename it to check.py, the class to CheckDetail, etc.

There was a reason for it in AutoQA. I guess it was caused by the fact that you could either run the tests under the autotest user (through the autotest server) or the local user (using local execution), and therefore we separated this out in order to be able to have the main config file world readable.

I don't know how this will work with buildbot. Will the test clients run the check under root or under a specified account? How are we going to install the appropriate config files - from RPM, or copy from the server?

It might be just easier to keep passwords separate from the main config file, but if you want, I can merge it.

IIUC @jskladan is working on bodhi commenting directive, therefore everything related to bodhi is still needed - that's [notifications] and [bodhi_email]. The rest of [resources] is obviously needed as well. I assume @jskladan can strip some config options if he end up not using them. Or I can remove them now and he can add them back. But I assumed it's needless work, because if he takes the code without large adjustments, pretty much all options should be needed. And for [paths], well, I assumed a log file location will be useful in general, once we hook it up in our logging framework.

I tried to strip down all autoqa-related pieces. What exactly do you mean?

I talked to @jskladan and I understood that different result keywords are not going away. TAP offers only "ok" and "not ok", but we will probably want to provide more result states, so that's why they are listed for easy reference. We also probably want some summary (TAP supports that) and of course output. Some additional keyvals might be useful for some checks, and @jskladan says ResultsDB still supports them.

I found CheckDetail class very useful for upgradepath check, or probably any multi-result check. The problem is that I need to track result/summary/output for many "items". I can of course create custom objects inside upgradepath to do this, but why shouldn't we provide a well-written class for these purposes with some convenience methods? For example the result keyword ordering is very helpful when deciding the final result for the item in question. The log() method is a very convenient way how to fill the ouput for such an item and print it out to stdout at the same time. I could remove create_multi_item_summary() method, it's not vital and it's just for pretty output when looking at the overall logs or watching interactively, but again I imagine it might be useful for more checks. The rest of the file is stripped to the bone, I think, just compare it with test.py in AutoQA.

My idea is to provide some convenience methods for converting CheckDetail (or multiple CheckDetails) to TAP result. That means if you use this container class for tracking check results, your TAP output will get created automagically with a single method call. Of course you should still be able to manually create TAP output by manually providing the required information, but I feel this could offer a nice integration. Use CheckDetail for tracking your check progress, then call CheckDetail.tap(). Do you have a different idea?

Have you seen line 27? :-)

I could probably covert it to CONF_DIR = [ 'location1', 'location2' ], if you prefer.

Using a Python script for configuration and then running eval() or execfile() on it seems quite unsafe to me. It would remove a lot of the complexity, that's for sure. Do you consider it a good trade-off?

I use ConfigParser and feed the cfg file paths to the constructor, that's the _load() method. The rest of the code deals with two major issues:

1. The defaults should be hardcoded to the application as well. I believe that if you comment out or delete any line in the cfg file, the application should not crash. That means you can have your keyvals only in the cfg file, you need to have it hardcoded in the application as well. It can be definitely split to a separate module (not like here), but that's not that important. So that's lines 81-111.
2. It's better to retype once than 100 times. If you use plain ConfigParser object, you need to use methods like getint() and getfloat() every time you want to access some configuration value. That means you need to have _lots of_ try-catch blocks all around your code. You need to handle the exception states reasonably well everywhere. That's a lot of code in each module. That's what we used to have in AutoQA in the old days. That's why I got the idea to retype it once, on the first config file use, and then just access objects in all the modules, without any need for error checking. All the defensive programming is in a single place, here. If something has a wrong value, you print an error and use the hardcoded default value. You don't need to implement it in every module using this value, you do it just once. That's lines 115-191 and 56-60.

There is a convenience object on lines 62-78 that auto-creates file paths. It can be removed, if we decide to call the makedirs() method after each assignment to such an object.

I'm fine with using a different approach, but I remember that we used plain ConfigParser objects and how many exception handlers we used to have scattered throughout the code. That's why we've come up with this.

If it increases clarity, I can at least separate out lines 81-111 into its own module. I can also probably strip some warnings from the documentation, they might be excessive.

Will do

Martin said he would introduce koji_utils in his own patch, so I'll remove this file for the moment.

Actually this operates on Koji objects, so it's very Koji specific. For plain strings, you can simply use rpmUtils module, as demonstrated on line 101. But if you need to compare ENVR from Koji objects, this is the method you need to use.

Again, it operates on Koji objects. Where would you like to see it?

The problem with Koji API is that it hides epoch by default, and you need to specifically ask for it (epoch handling feels like an afterthought in most of Fedora tools). But epoch is really important very often. We could provide a patch for upstream koji module to add methods like this, and we wouldn't need it here then.

See my response above. If we use Python scripts for configuration, yes, all of this can go away. If we use ConfigParser, well, you need a way to convert strings to booleans, because:

In [2]: bool('False')
Out[2]: True

Of course we can get rid of yes/no, on/off etc support. But that doesn't add too much complexity on top of True/False parsing, I think.

Might be even fedora_repos.py or something like that. Roger.

I can do that. However, please note that config.py also reads from config files on import. And even if it didn't, any module using config objects would trigger it as well (for example koji or bodhi utils, they read config file to learn the default Koji/Bodhi server address). The only way would be to create all objects manually (instead of from bodhi_utils import bodhi you'd have to use bodhi = bodhi_utils.getBodhiClient()) and avoid using config values references in method arguments (instead you'd initialize the values dynamically in method body). It would also add new methods like getBodhiClient() instead just using a pre-made attribute.

Why exactly do you consider reading a config file at import time to be a problem?

Actually I assumed this module can contain system specific functions from different operating systems (like rpm methods or deb methods). I forgot to include a module docstring, sorry about that. I can still rename it, but I don't expect too many rpm utils in here.

What exactly does "autoqa-ish" mean, and why is it a bad thing? ;)

But as @kparal said above, I do believe that this CheckDetail is quite usefull. IIRC in AutoQA it produced the plaintext and HTML representation of results, hence I see no problem in it providing the TAP output. Also, it is IMHO much better to have this one common 'result' container, from which we can then create TAP/HTML/whatever reports/outputs. And I'd rather use this, then directly calling mcepls TAP-producer library. I'm not saying we are getting rid of it any time soon (or ever), but if we ever do, I'd rather have to change one library, then all the tests.

That might actually be better, it took me two readings to notice all the possible sources for the config files.

I'd vote for exceptions.py.

Just as a side-note, though from the top of my head, SqlAlchemy uses exc.py.

Ad testability - all it needs to be easily testable is (IMHO) changing the:

​class TaskotronKojiClient(koji.ClientSession):
    def __init__(self, server=Config.resources.koji_url, opts=None):
        koji.ClientSession.__init__(self, server, opts)

to something like

​class TaskotronKojiClient(object):
    def __init__(self, koji_client = None):
        self.koji_client = koji_client
        if koji_client is None:
            self.koji_client =  koji.ClientSession.__init__(self, Config.resources.koji_url)

and changing the self.XYZ koji-client calls to self.koji_client.XYZ calls.

Then it will be quite simple to provide a Mock object as te koli client.

I'm not really a big fan of using 'scripts' for end-user configuration. I'm not that concerned about security, but IMHO having python script in /etc can be a slippery slope leading (at best) to "magical configuration".

Another option would be to use YAML for configuration. It supports basic types, therefore we wouldn't need to retype everything to a correct value. However, it's easy in YAML to change type by mistake (a typo in False makes it string rather than boolean), so we would still need to check for correct types during config load. Because, of course, we want to detect the problem ASAP rather than crash in some module during execution or seeing some strange side-effects. So, unfortunately, the methods in config.py would not be reduced. But this method (getbool()) could go away. Would you like to see YAML instead of ini config files?

For some reason, I read over this the first time around. This shouldn't be created @ import and should be created at first usage.

If code elsewhere is using the raw BodhiClient, it should be refactored to use bodhi_utils functions/methods instead. The primary purpose of bodhi_utils should be to contain our interactions with bodhi and to provide a consistant interface for getting the data we need from bodhi.

What exactly does "autoqa-ish" mean, and why is it a bad thing? ;)

I'm not saying that anything from AutoQA is bad, just that this seemed like a crutch to avoid integrating the old checks with the new system - it sounds like I may have been missing parts of the puzzle, though.

Is there a task for getting TAP out of the CheckDetail object? Is there a mapping from the autoqa statuses to TAP statuses?

I like the idea of having an intermediary result-ish object but I think this will need work before using it for much.

Have you seen line 27? :-)

Whoops. Missed that.

I could probably covert it to CONF_DIR = [ 'location1', 'location2' ], if you prefer.

I agree with @jskladan, that would be easier to read

Apologies, I should have been more verbose about what I was thinking WRT using python scripts. I agree with the idea of not doing getint() and having extra parsing logic everywhere in our code but I have some specific concerns about this exact methodology.

Using a Python script for configuration and then running eval() or execfile() on it seems quite unsafe to me. It would remove a lot of the complexity, that's for sure. Do you consider it a good trade-off?

As I said elsewhere in the review, I'm not incredibly picky about whether we use ConfigParser, YAML or python for configuration retrieval from files.

The main concerns I have about this code are:

Unit Testing

With the code as is, it's difficult to create configuration without touching the filesystem. Unit tests should not touch the filesystem.

Doing non-config stuff in config

There is a convenience object on lines 62-78 that auto-creates file paths. It can be removed, if we decide to call the makedirs() method after each assignment to such an object.

I don't like having tmpdir creation in the config - in my mind, it should be handled separately and at most, the config should contain a basedir for tempdirs and/or information on naming conventions. Is there a reason we started doing this in AutoQA?

Mixing of defaults, data member access and parsing in the same code

I'm not a huge fan of how all the defaults are mixed in with the configuration access methods and parsing. Would you be ok with separating things out as:

• a config class which stores the config values as accessed in taskotron
  • defaults stored either in the config file format or as the default values of that class/object
• configfile parsing code which reads in some input (ini, yaml etc.) and populates/creates a config object

What I would like to see is:

• the ability to have different defaults for different running modes (production, testing, dev etc.)
• separation of concerns (tmpdir creation in file_utils or a tmpdir_utils module)
• the ability to create a config object for unit tests without touching the filesystem

I missed this before - I'd prefer not to rely on urlgrabber going forward. It's error output is not great and IIRC, it has no python3 plans. If we do use it for now, let's file a task to replace it with requests.

For reference @mkrizek submitted D17 for review which covers most of my concerns about testing and testability

Actually this operates on Koji objects, so it's very Koji specific. For plain strings, you can simply use rpmUtils module, as demonstrated on line 101. But if you need to compare ENVR from Koji objects, this is the method you need to use.

I dislike the duplication of logic. I understand that koji has it's own way of doing things but I'd prefer to see our code using getENVR() on the koji data object and rpmUtils for the envr comparison instead of synthesizing the envr of a build in multiple places. If we end up using an envr comparison method in rpm_utils (what is currently system_utils), I'd also rather use that than have separate calls to rpmUtils

Point taken in this case. I would like to see the function naming convention match the other functions in koji_utils, though. get_envr instead of getENVR for stylistic consistency

I'm not really a big fan of using 'scripts' for end-user configuration. I'm not that concerned about security, but IMHO having python script in /etc can be a slippery slope leading (at best) to "magical configuration".

Yeah, it'd require discipline to not do stupid things if we went that route. An example of what I had in mind is in the blockerbugs app.

Default values are stored in config.py which allows for different default settings in production, dev and testing modes. These defaults can be overridden with values in /etc/blockerbugs/settings.py or ./conf/settings.py.

Of course we can get rid of yes/no, on/off etc support. But that doesn't add too much complexity on top of True/False parsing, I think.

I'm not a huge fan of too many values for booleans in config files. I'd rather see case insensitive parsing of true/false but it's nothing critical.

I don't have strong feelings on which mechanism is used for configuration but I would like to see the ability to have different defaults for running locally, running in production and running in dev/debug mode.

Good idea, I like fedora_repos

I can do that. However, please note that config.py also reads from config files on import. And even if it didn't, any module using config objects would trigger it as well (for example koji or bodhi utils, they read config file to learn the default Koji/Bodhi server address).

I had concerns about that as well but didn't do a good job of voicing them the first time through. I've added more detail on the config stuff this time, though.

The only way would be to create all objects manually (instead of from bodhi_utils import bodhi you'd have to use bodhi = bodhi_utils.getBodhiClient()) and avoid using config values references in method arguments (instead you'd initialize the values dynamically in method body). It would also add new methods like getBodhiClient() instead just using a pre-made attribute.

I'd actually argue that there are few use cases where code should be getting a BodhiClient object from bodhi_utils. By using that object in multiple places, we increase the number of places which would be affected by any changes in the bodhi client code or our usage of that client. It would be better to have a bodhi_utils.query() callable and other interface to isolate our interactions with bodhi and the bodhi client into one module.

Why exactly do you consider reading a config file at import time to be a problem?

Testability. Technically, you could monkeypatch out the actions @ import but that doesn't stop the objects from being created, files from being read and network connections from being created. It also makes for messier, more complex and slower unit tests.

Stuff like creating a bodhi client on import are worse in my mind, especially if other pieces of code are using that raw object. It tightens coupling between our code and bodhi in additon to making that code more difficult to isolate in unit tests.