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(a)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
> 1)
> 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:
1. 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.
2. 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