On Wed, 2010-12-15 at 12:11 -0500, Kamil Paral wrote:
Replying for Martin, since he's long gone (at school, probably) :)
----- "James Laska" jlaska@redhat.com wrote:
diff --git a/autoqa b/autoqa index fd41503..190e63b 100755 --- a/autoqa +++ b/autoqa @@ -40,12 +40,14 @@ conf = { 'testdir': '/usr/share/autotest/client/site_tests', 'hookdir': '/usr/share/autoqa', 'notification_email': '',
- 'send_notification_email': 'false',
It feels a little weird having two ways to disable email notification
empty notication_email='' or 2) send_notification_email=false. I'd prefer just the first one since that seems simpler. Were the occasions where having send_notification_email *and* notification_email was needed?
send_notification_email is used just to disable notification emails (so you can keep your notification_email values defined). True, this may be a little overkill, it depends.
The idea was to have a set of options send_* which you can set to False or True and therefore selectively disable/enable parts of the notification system (emails, Bodhi comments, etc).
But it's good to receive similar feedback and step back a moment. Sure, we can simplify it (revert it back) to just one option. That also holds for "send_result_email" probably.
My comment was definitely more of a "style" comment than anything functional. It drives me nuts when we overload the meaning of something (which I do all the time btw). For this case, it doesn't seem like we are overloading the meaning of 'notification_email' (and related values) by having them empty, or a comma-separated list. I'd opt for fewer values rather than similarly named values for the different functions. I have a feeling the additional true/false values would trick me when tuning the configuration in our public instance (may just be a weird jlaska brain thing) :)
+# The 'resources' section specify access details to various
external services
+[resources] +# URL of Koji instance used for querying about new builds +koji_url = http://koji.fedoraproject.org/kojihub +# URL of repository of all the RPM packages built in Koji +pkg_url = http://koji.fedoraproject.org/packages +# URL of Bodhi instance used for communication about package
updates
+# (leave blank for default) +bodhi_server =
To avoid having too many config sections, should we just move these into [general]? Or is there some thinking about how this section might grow over time?
It will surely grow ResultsDB URL very soon. I don't know about other services.
We can move everything to "general" section :) From a source code perspective, it doesn't really matter if we have one or more sections. From a system admin perspective, it seemed to me that having several sections makes the config file clearer to understand. But, again, no hard feelings in here.
Oh I understand now, hah I just had to read the comment above [resources]. That makes total sense now. I was hoping to avoid adding sections for the sake of adding sections. But there was clearly more thought put into this.
diff --git a/hooks/post-koji-build/watch-koji-builds.py
b/hooks/post-koji-build/watch-koji-builds.py
index 4cc2d7f..57f0aee 100755 --- a/hooks/post-koji-build/watch-koji-builds.py +++ b/hooks/post-koji-build/watch-koji-builds.py @@ -41,9 +41,6 @@ try: except OSError, e: if e.errno != 17: # already exists raise -# XXX configparser? /etc/koji.conf, section 'koji', item 'server' -# alternately: read from e.g. /etc/autoqa/autoqa.conf -kojiserver = 'http://koji.fedoraproject.org/kojihub'
I like the goal of this change, consolidating config handling etc... I might offer that we still need kojiserver in this script for testing purposes when calling watch-koji-builds.py by hand. Let me try to explain my thoughts ...
I *think* we want the callers to be responsible for 1) parsing arguments, 2) loading the configuration default options, and 3) passing it to the underlying modules. So for example, the autoqa script and the other callable .py scripts (e.g. watch-koji-builds.py) are responsible for processing command-line arguments, loading configuration defaults (autoqa.conf) and passing that on to the appropriate modules. I could be wrong, and it might not look appropriate when done ... but I wonder if ...
1. Add an --koji_server argument to watch-koji-builds.py so the caller can specify a different server 2. The --koji_server argument will default to the value
specified in autoqa.conf 3. watch-koji-builds.py passes the value of --koji_server to koji_utils.SimpleKojiClientSession()
Does this make sense?
It makes perfect sense. I just didn't see any use case for using different Koji server (different from the one defined in autoqa.conf) when running the watcher. I usually try to implement features only when I know that I need them (the "keep it simple" principle). So I was considering the exactly same thing as you propose, but I ended with "We'll do that when we need that." What is our use case for that command-line option?
This is probably more of a personal "style" preference again. I tend to prefer having command-line overrides for config file defaults when doing this sort of thing. You're right that we should let the use case determine the urgency/importance of this approach. If we had the ability to specify the server before, let's not remove that support. If the support didn't exist ... someone can always add that in their free time!
For the use case ... I typically run the watcher scripts by hand when testing stuff. When testing, I find it really helpful to be able to run the script with options, rather than changing the stock config file and then changing it back when I'm done.
--- a/lib/python/bodhi_utils.py +++ b/lib/python/bodhi_utils.py @@ -28,6 +28,32 @@ from datetime import datetime from util import SingleConfigParser, getbool import ConfigParser
+try:
- autoqa_conf = SingleConfigParser()
- autoqa_conf.read_single(['autoqa.conf',
'/etc/autoqa/autoqa.conf'])
- server = autoqa_conf.get('resources', 'bodhi_server')
+except ConfigParser.Error, e:
- server = ''
I'm not a fan of specifying config file paths in multiple places. Ideally, just <one module would handle all config file loading, instantiate the config object, and all other modules would use that object for access.
This is https://fedorahosted.org/autoqa/ticket/255. Certainly needed soon, because as our libraries get more complex, we start to repeat the config parsing code over and over again. Agreed.
That's the ticket! Oops, I linked to the wrong one.
Long-term, we're moving this over to autoqa/config.py (or similar) to handle specifying the config file paths, right (https://fedorahosted.org/autoqa/ticket/253)?
Actually, ticket #253 is about something little different - ensuring that all config files are loaded primarily from CWD and only after that from /etc. I have it almost done.
That's included in Martin's patch right? At least I support for accepting multiple conf files, but reading only one.
The above would then change to ...
from autoqa.config import autoqa_cfg server = autoqa_cfg.get('resources', 'bodhi_server', '')
There are two issues mixed:
- Do not repeat the parsing code - that is ticket #255. It is not a one-liner and should not be part of this patch (let's keep issues separate, this patch is about supporting staging server).
Sounds good.
- Do not create ConfigParser instance with the same file paths as input over and over again in all modules - that is what "from autoqa.config import autoqa_cfg" solves. That is a one-liner and can be part of this patch, if you think it's appropriate. Ticket #255 is a superset of this problem and will replace this solution eventually. I've become so obsessed with "The One And True Solution" that I forgot we can do smaller steps in the meantime :)
Haha ... nope, you're correct in the order. I know we talked about that ticket, but wasn't sure where it fit into the mix. That'll definitely help cleanup the config handling.
--- a/lib/python/koji_utils.py +++ b/lib/python/koji_utils.py @@ -24,15 +24,16 @@ import koji from repoinfo import repoinfo import rpmUtils.miscutils import sys
-# XXX fetch from /etc/koji.conf, section 'koji' -kojiurl = 'http://koji.fedoraproject.org/kojihub' -pkgurl = 'http://koji.fedoraproject.org/packages' +from autoqa.bodhi_utils import SingleConfigParser
class SimpleKojiClientSession(koji.ClientSession): '''Convenience wrapper class for interacting with koji'''
- def __init__(self, server=kojiurl, opts=None):
return koji.ClientSession.__init__(self, server, opts)
- def __init__(self, opts=None):
cfg_parser = SingleConfigParser()
cfg_parser.read_single(['autoqa.conf',
'/etc/autoqa/autoqa.conf'])
Same as earlier, too much duplication of config filepaths. Can we stuff this into autoqa/config.py to handle for us?
Yes, good idea.
Thanks, James
Thank you for your review. Let's keep Martin on his toes, he likes more work :)
Thank you for the time and effort adding this support! :)
Thanks, James