[PATCH] Add support for a staging server.

[Martin Krizek]

Hey all,

this patch solves ticket #241. It adds options in autoqa.conf to turn on/off
optin, notifications and results e-mails. There are options containing urls of
koji and bodhi instances that will be used.

Also there are some alterations in bodhi_utils, there is only one creation of
BodhiClient instance and one config files reading to simplify the code.

You can find the patch in mkrizek-staging-ready branch.

---
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',
     'autotest_server': socket.gethostname(),
 }
 
 cfg_parser = SingleConfigParser()
 cfg_parser.read(cfgfile)
-conf = cfg_parser.get_section('general', conf) # used by prep_controlfile
+conf = cfg_parser.get_section('general', conf)
+conf = cfg_parser.get_section('notifications', conf)
 # we don't need to catch errors here, bcz we want autoqa to crash for invalid config
 
 def prep_controlfile(controlfile, extradata):
@@ -283,7 +285,10 @@ for test in testlist:
 
     for arch in test_vars[test]['archs']:
         testname='%s:%s.%s' % (hookname, test, arch)
-        email = conf['notification_email']
+        if conf['send_notification_email']:
+            email = conf['notification_email']
+        else:
+            email = ''
 
         if run_local:
             retval = run_test_locally(control, name=testname,
diff --git a/autoqa.conf b/autoqa.conf
index 69278fe..886536e 100644
--- a/autoqa.conf
+++ b/autoqa.conf
@@ -6,29 +6,62 @@
 hookdir = /usr/share/autoqa
 # Path containing the actual autoqa tests to be run
 testdir = /usr/share/autotest/client/site_tests
-# comma-separated email addresses that will get notification of test completion
-# (leave blank to disable)
-notification_email =
 # Set to "true" if you want autoqa to always run tests locally using
'autotest'.
 # Default is to schedule jobs with the autotest server using 'atest'.
 # This is the same as using 'autoqa --local'.
-#local = true
+local = false
+# Hostname of this autotest server. Used for constructing URLs for accessing
+# test results. By default current hostname is used.
+autotest_server =
 
-# The 'test' section is used for systemwide configuration of autoqa tests
-[test]
-# comma-separated list of email addresses that will receive full test results
-# (leave blank to disable)
-result_email =
-# email address for From: line of emails sent by tests
-mail_from = autoqa(a)fedoraproject.org
-# hostname or hostname:port of smtp server / mailhub to use for sending email
+# The 'notifications' section controls which notifications about which events
+# you want to send
+[notifications]
+# Sender email address used for all emails sent from AutoQA.
+mail_from =
+# Hostname or hostname:port of SMTP server used for sending emails.
+# Please note that the emails are sent from the autotest clients, not the
+# server.
 smtpserver = localhost
 
-[bodhi]
-# If "true", test results (for tests utilizing this feature) will be sent
-# as comments to Fedora Update System (Bodhi). This requires that you have
-# Bodhi credentials filled in in fas.conf.
+# Comma-separated list of email addresses that will receive full test results
+# of every completed test.
+result_email =
+# If "true", result email will be sent. Otherwise "false".
+send_result_email = false
+
+# Opt-in emails are sent (for selected tests) to the maintainers of a package
+# that has just been tested and the maintainer opted in. Read more at:
+# http://jlaska.wordpress.com/2010/06/01/fedora-package-maintainers-want-te...
+# If "true", opt-in email will be sent. Otherwise "false".
+send_optin_email = false
+
+# Comma-separated email addresses that will get notification of every job
+# completion or every job state change as defined in 'notify_email_statuses'
+# in ~autotest/global_config.ini.
+# Important: These emails are handled and sent by autotest server, the
+# 'mail_from' and 'smtpserver' options don't apply for it.
+# Read more at http://autotest.kernel.org/wiki/GlobalConfig (search for
+# 'notify_email').
+notification_email =
+# If "true", notification email will be sent. Otherwise "false".
+send_notification_email = false
+
+# If "true", test results (for selected tests) will be sent as comments to
+# Fedora Update System (Bodhi). This requires that you have Bodhi credentials
+# filled in in fas.conf.
 send_bodhi_comments = false
-# how long (minutes) should we wait before posting the same comment to bodhi
-# by default 3 days (3*24*60 = 4320)
+# How long (in minutes) should we wait before allowing consequent test to
+# re-post a 'FAILED' comment into Bodhi once again.
+# By default 3 days (3*24*60 = 4320).
 bodhi_posting_comments_span = 4320
+
+# 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 =
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'
 
 def get_prevtime():
     '''Returns the prevtime to use when looking for new builds.  Value will
be
@@ -126,7 +123,7 @@ tags for new builds and kick off tests when new builds/packages are
found.')
 
     # Set up the koji connection
     kojiopts = {} # Possible items: user, password, debug_xmlrpc, debug..
-    session = koji_utils.SimpleKojiClientSession(kojiserver, kojiopts)
+    session = koji_utils.SimpleKojiClientSession(kojiopts)
     untagged_builds = []
     tagged_builds = []
     exit_status = 0
diff --git a/lib/python/bodhi_utils.py b/lib/python/bodhi_utils.py
index 94e1a7d..95d18a5 100644
--- 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 = ''
+
+try:
+    fas_conf = SingleConfigParser()
+    fas_conf.read_single(['fas.conf', '/etc/autoqa/fas.conf'])
+    fas = fas_conf.get_section('fas')
+    user = fas['username']
+    pswd = fas['password']
+    if not user or not pswd:
+        raise IOError
+except (IOError, ConfigParser.Error), e:
+    # if there's no fas.conf or it's incomplete, we just log in anonymously.
+    user = ''
+    pswd = ''
+
+if server:
+    bodhi = fedora.client.bodhi.BodhiClient(base_url=server, username=user,
password=pswd)
+else:
+    bodhi = fedora.client.bodhi.BodhiClient(username=user, password=pswd)
+
 def bodhitime(timestamp):
     '''Convert timestamp (seconds since Epoch, assumed to be local time) to
a
     Bodhi-approved time string ('%Y-%m-%d %H:%M:%S')'''
@@ -40,7 +66,6 @@ def parse_bodhitime(bodhitimestr):
 
 def bodhi_list(params, limit=100):
     '''Perform a bodhi 'list' method call, with pagination
handling'''
-    bodhi = fedora.client.bodhi.BodhiClient()
     params['tg_paginate_limit'] = limit
     params['tg_paginate_no'] = 1
 
@@ -136,7 +161,7 @@ def _is_bodhi_testresult_needed(old_result, comment_time, result,
time_span):
 
     return True
 
-def bodhi_post_testresult(update, testname, result, url, config, arch = 'noarch',
karma = 0):
+def bodhi_post_testresult(update, testname, result, url, arch = 'noarch', karma =
0):
     '''Post comment and karma to bodhi
 
        Args:
@@ -144,7 +169,6 @@ def bodhi_post_testresult(update, testname, result, url, config, arch
= 'noarch'
        testname -- the name of the test
        result -- the result of the test
        url -- url of the result of the test
-       config -- autoqa config
        arch -- tested architecture (default 'noarch')
        karma -- karma points (default 0)
 
@@ -153,37 +177,25 @@ def bodhi_post_testresult(update, testname, result, url, config,
arch = 'noarch'
        posted (either posting is turned off or comment was already posted),
        False otherwise.
     '''
+
+    # TODO when new bodhi releases, add update identification by UPDATEID support
+
     err_msg = 'Could not post a comment to bodhi'
-    conffiles = ['fas.conf', '/etc/autoqa/fas.conf']
-    try:
-        cfg_parser = SingleConfigParser()
-        cfg_parser.read_single(conffiles)
-        fas = cfg_parser.get_section('fas')
-    except (IOError, ConfigParser.Error), e:
-        print >> sys.stderr, "ERROR: Couldn't read none of config files:
%s" % conffiles
-        print >> sys.stderr, e
-        return False
 
     if not update or not testname or not result or url == None:
         sys.stderr.write('Incomplete arguments!\n%s\n' % err_msg)
         return False
 
     try:
-        if not getbool(config.get('bodhi', 'send_bodhi_comments')):
-            print 'Sending bodhi comments is turned off. Test result will NOT be
sent.'
-            return True
-    except KeyError:
+        if not getbool(autoqa_conf.get('notifications',
'send_bodhi_comments')):
+            raise ValueError
+    except ValueError:
         print 'Sending bodhi comments is turned off. Test result will NOT be
sent.'
-        # option missing -> it's false, do not send it (but return True since
+        # it's either False or not a bool -> it's false, do not send it (but
return True since
         # it's intentional, not an error)
         return True
 
-    try:
-        user = fas['username']
-        pswd = fas['password']
-        if not user or not pswd:
-            raise KeyError
-    except KeyError:
+    if not user or not pswd:
         sys.stderr.write('Conf file containing FAS credentials is
incomplete!\n%s\n' % err_msg)
         return False
 
@@ -191,14 +203,12 @@ def bodhi_post_testresult(update, testname, result, url, config,
arch = 'noarch'
                % (testname, result, arch, url)
     try:
         (old_result, comment_time) = _bodhi_already_commented(update, user, testname,
arch)
-        time_span = int(config.get('bodhi',
'bodhi_posting_comments_span'))
+        time_span = int(autoqa_conf.get('notifications',
'bodhi_posting_comments_span'))
 
         if not _is_bodhi_testresult_needed(old_result, comment_time, result, time_span):
             print 'The test result already posted to bodhi.'
             return True
 
-        bodhi = fedora.client.BodhiClient(username=user, password=pswd)
-
         if not bodhi.comment(update, comment, karma):
             sys.stderr.write('%s\n') % err_msg
             return False
@@ -216,10 +226,7 @@ def _self_test():
     Simple self test.
     '''
     from datetime import timedelta
-    from ConfigParser import SafeConfigParser
-    cfg_parser = SafeConfigParser()
-    cfg_parser.read('/etc/autoqa/autoqa.conf')
-    time_span = int(cfg_parser.get('bodhi',
'bodhi_posting_comments_span'))
+    time_span = int(autoqa_conf.get('notifications',
'bodhi_posting_comments_span'))
     try:
         print '1. Test:',
         assert _is_bodhi_testresult_needed('PASSED', datetime.now,
'PASSED', time_span) == False
diff --git a/lib/python/koji_utils.py b/lib/python/koji_utils.py
index 87b32e9..648412b 100644
--- 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'])
+        self.server = cfg_parser.get('resources', 'koji_url')
+        self.pkgurl = cfg_parser.get('resources', 'pkg_url')
+        return koji.ClientSession.__init__(self, self.server, opts)
 
     def list_builds_since(self, timestamp):
         '''Return a list of new builds since the given
timestamp'''
@@ -147,7 +148,7 @@ class SimpleKojiClientSession(koji.ClientSession):
 
     def nvr_to_urls(self, nvr, arches=None, debuginfo=False):
         info = self.getBuild(nvr)
-        baseurl = '/'.join((pkgurl, info['package_name'],
+        baseurl = '/'.join((self.pkgurl, info['package_name'],
                            info['version'], info['release']))
 
         rpms = self.listRPMs(buildID=info['id'], arches=arches)
@@ -162,7 +163,7 @@ class SimpleKojiClientSession(koji.ClientSession):
         key added with URL where to download the RPM as the value.'''
 
         info = self.getBuild(nvr)
-        baseurl = '/'.join((pkgurl, info['package_name'],
+        baseurl = '/'.join((self.pkgurl, info['package_name'],
                            info['version'], info['release']))
 
         rpms = self.listRPMs(buildID=info['id'])
diff --git a/lib/python/test.py b/lib/python/test.py
index 29bb96c..73efc80 100644
--- a/lib/python/test.py
+++ b/lib/python/test.py
@@ -21,7 +21,7 @@ from autotest_lib.client.bin import test, utils
 from autotest_lib.client.bin.test_config import config_loader
 
 from decorators import ExceptionCatcher
-from util import make_autotest_url
+from util import make_autotest_url, getbool
 import os, sys, traceback
 
 class AutoQATest(test.test, object):
@@ -80,7 +80,7 @@ class AutoQATest(test.test, object):
 
         # prepare email
         send_to = []
-        result_email = self.config.get('test','result_email')
+        result_email = self.config.get('notifications','result_email')
         if result_email and isinstance(result_email, str):
             send_to = result_email.split(',')
             try:
@@ -114,12 +114,12 @@ class AutoQATest(test.test, object):
         log.close()
 
         # send email
-        if send_to:
+        if getbool(self.config.get('notifications', 'send_result_email'))
and send_to:
             utils.send_email(mail_to=send_to,
-                    mail_from=self.config.get('test','mail_from'),
-                    mail_server=self.config.get('test','smtpserver'),
-                    subject=self.summary,
-                    body = mail_body)
+                mail_from=self.config.get('notifications','mail_from'),
+               
mail_server=self.config.get('notifications','smtpserver'),
+                subject=self.summary,
+                body = mail_body)
 
     def _convert_list_variables(self):
         '''
diff --git a/tests/initscripts/initscripts.py b/tests/initscripts/initscripts.py
index 8e35e35..097fbc8 100644
--- a/tests/initscripts/initscripts.py
+++ b/tests/initscripts/initscripts.py
@@ -43,7 +43,7 @@ class initscripts(AutoQATest):
             f = open("/etc/yum.repos.d/beakerlib.repo", "w")
             f.write("[beakerlib-testing]\nname=Testing repo for
BeakerLib\nbaseurl=http://afri.fedorapeople.org/beakerlib/\nenabled=1\ngp...)
             f.close()
-        utils.system('yum -y install beakerlib') 
+        utils.system('yum -y install beakerlib')
 
     @ExceptionCatcher()
     def initialize(self, config, **kwargs):
@@ -174,7 +174,9 @@ class initscripts(AutoQATest):
 
             # email results to mailing list and to pkg owner if they optin
             repo = repoinfo.getrepo_by_tag(kojitag)
-            if repo is not None and autoqa.util.check_opt_in(name,
repo['collection_name']):
+            send_optin_email = getbool(self.config.get('notifications',
'send_optin_email'))
+            if repo is not None and send_optin_email and \
+            autoqa.util.check_opt_in(pkg_name, repo['collection_name']):
                 #FIXME - hardcoded partial email address here - obviously sub-par
                 self.mail_to.append('%s-owner(a)fedoraproject.org' % name)
 
diff --git a/tests/rpmguard/rpmguard.py b/tests/rpmguard/rpmguard.py
index bf973f0..4a20d53 100644
--- a/tests/rpmguard/rpmguard.py
+++ b/tests/rpmguard/rpmguard.py
@@ -82,7 +82,9 @@ class rpmguard(AutoQATest):
             # email results to mailing list and to pkg owner if they optin
             repo = repoinfo.getrepo_by_tag(kojitag)
             pkg_name = rpmUtils.miscutils.splitFilename(envr + '.noarch')[0]
-            if repo is not None and autoqa.util.check_opt_in(pkg_name,
repo['collection_name']):
+            send_optin_email = getbool(self.config.get('notifications',
'send_optin_email'))
+            if repo is not None and send_optin_email and \
+            autoqa.util.check_opt_in(pkg_name, repo['collection_name']):
                 #FIXME - hardcoded partial email address here - obviously sub-par
                 self.mail_to.append('%s-owner(a)fedoraproject.org' % pkg_name)
 
diff --git a/tests/rpmlint/rpmlint.py b/tests/rpmlint/rpmlint.py
index 1c06a8b..cac8a48 100644
--- a/tests/rpmlint/rpmlint.py
+++ b/tests/rpmlint/rpmlint.py
@@ -116,7 +116,9 @@ class rpmlint(AutoQATest):
             # email results to mailing list and to pkg owner if they optin
             repo = repoinfo.getrepo_by_tag(kojitag)
             pkg_name = rpmUtils.miscutils.splitFilename(envr + '.noarch')[0]
-            if repo is not None and autoqa.util.check_opt_in(pkg_name,
repo['collection_name']):
+            send_optin_email = getbool(self.config.get('notifications',
'send_optin_email'))
+            if repo is not None and send_optin_email and \
+            autoqa.util.check_opt_in(pkg_name, repo['collection_name']):
                 #FIXME - hardcoded partial email address here - obviously sub-par
                 self.mail_to.append('%s-owner(a)fedoraproject.org' % pkg_name)
 
-- 
1.7.3.2

---

Thanks,
Martin

[James Laska]

On Wed, 2010-12-15 at 08:25 -0500, Martin Krizek wrote:
>  Hey all,
>  
>  this patch solves ticket #241. It adds options in autoqa.conf to turn on/off
>  optin, notifications and results e-mails. There are options containing urls of
>  koji and bodhi instances that will be used.
>  
>  Also there are some alterations in bodhi_utils, there is only one creation of
>  BodhiClient instance and one config files reading to simplify the code.
>  
>  You can find the patch in mkrizek-staging-ready branch. 
Thanks Martin!

I'm a bit wordy in my comments, but the general theme around avoiding
potentially duplicate autoqa.conf options and figuring out a consistent
approach for how we want to handle cli arguments, load autoqa.conf
defaults and where's the best place for that code to live.

>  ---
>  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?

>       'autotest_server': socket.gethostname(),
>   }
>   
>   cfg_parser = SingleConfigParser()
>   cfg_parser.read(cfgfile)
>  -conf = cfg_parser.get_section('general', conf) # used by prep_controlfile
>  +conf = cfg_parser.get_section('general', conf)
>  +conf = cfg_parser.get_section('notifications', conf)
>   # we don't need to catch errors here, bcz we want autoqa to crash for invalid
> config
>   
>   def prep_controlfile(controlfile, extradata):
>  @@ -283,7 +285,10 @@ for test in testlist:
>   
>       for arch in test_vars[test]['archs']:
>           testname='%s:%s.%s' % (hookname, test, arch)
>  -        email = conf['notification_email']
>  +        if conf['send_notification_email']:
>  +            email = conf['notification_email']
>  +        else:
>  +            email = '' 
This depends on whether we use multiple configuration options to enable
email notification.  If we continue with the single option, I'm fond of
adding a default value when loading a value from a dictionary ...

email = conf.get('notification_mail', '')

>           if run_local:
>               retval = run_test_locally(control, name=testname,
>  diff --git a/autoqa.conf b/autoqa.conf
>  index 69278fe..886536e 100644
>  --- a/autoqa.conf
>  +++ b/autoqa.conf
>  @@ -6,29 +6,62 @@
>   hookdir = /usr/share/autoqa
>   # Path containing the actual autoqa tests to be run
>   testdir = /usr/share/autotest/client/site_tests
>  -# comma-separated email addresses that will get notification of test completion
>  -# (leave blank to disable)
>  -notification_email =
>   # Set to "true" if you want autoqa to always run tests locally using
> 'autotest'.
>   # Default is to schedule jobs with the autotest server using 'atest'.
>   # This is the same as using 'autoqa --local'.
>  -#local = true
>  +local = false
>  +# Hostname of this autotest server. Used for constructing URLs for accessing
>  +# test results. By default current hostname is used.
>  +autotest_server =
>   
>  -# The 'test' section is used for systemwide configuration of autoqa tests
>  -[test]
>  -# comma-separated list of email addresses that will receive full test results
>  -# (leave blank to disable)
>  -result_email =
>  -# email address for From: line of emails sent by tests
>  -mail_from = autoqa(a)fedoraproject.org
>  -# hostname or hostname:port of smtp server / mailhub to use for sending email
>  +# The 'notifications' section controls which notifications about which events
>  +# you want to send
>  +[notifications]
>  +# Sender email address used for all emails sent from AutoQA.
>  +mail_from =
>  +# Hostname or hostname:port of SMTP server used for sending emails.
>  +# Please note that the emails are sent from the autotest clients, not the
>  +# server.
>   smtpserver = localhost
>   
>  -[bodhi]
>  -# If "true", test results (for tests utilizing this feature) will be sent
>  -# as comments to Fedora Update System (Bodhi). This requires that you have
>  -# Bodhi credentials filled in in fas.conf.
>  +# Comma-separated list of email addresses that will receive full test results
>  +# of every completed test.
>  +result_email =
>  +# If "true", result email will be sent. Otherwise "false".
>  +send_result_email = false
>  +
>  +# Opt-in emails are sent (for selected tests) to the maintainers of a package
>  +# that has just been tested and the maintainer opted in. Read more at:
>  +# http://jlaska.wordpress.com/2010/06/01/fedora-package-maintainers-want-te...
>  +# If "true", opt-in email will be sent. Otherwise "false".
>  +send_optin_email = false
>  +
>  +# Comma-separated email addresses that will get notification of every job
>  +# completion or every job state change as defined in 'notify_email_statuses'
>  +# in ~autotest/global_config.ini.
>  +# Important: These emails are handled and sent by autotest server, the
>  +# 'mail_from' and 'smtpserver' options don't apply for it.
>  +# Read more at http://autotest.kernel.org/wiki/GlobalConfig (search for
>  +# 'notify_email').
>  +notification_email =
>  +# If "true", notification email will be sent. Otherwise "false".
>  +send_notification_email = false 
Same as earlier, it seems weird to have two switches to enable/disable
email notification.

>  +# If "true", test results (for selected tests) will be
> sent as comments to
>  +# Fedora Update System (Bodhi). This requires that you have Bodhi credentials
>  +# filled in in fas.conf.
>   send_bodhi_comments = false
>  -# how long (minutes) should we wait before posting the same comment to bodhi
>  -# by default 3 days (3*24*60 = 4320)
>  +# How long (in minutes) should we wait before allowing consequent test to
>  +# re-post a 'FAILED' comment into Bodhi once again.
>  +# By default 3 days (3*24*60 = 4320).
>   bodhi_posting_comments_span = 4320
>  
>  +# 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?

>  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?

>   def get_prevtime():
>       '''Returns the prevtime to use when looking for new builds.  Value will
> be
>  @@ -126,7 +123,7 @@ tags for new builds and kick off tests when new builds/packages are
> found.')
>   
>       # Set up the koji connection
>       kojiopts = {} # Possible items: user, password, debug_xmlrpc, debug..
>  -    session = koji_utils.SimpleKojiClientSession(kojiserver, kojiopts)
>  +    session = koji_utils.SimpleKojiClientSession(kojiopts)
>       untagged_builds = []
>       tagged_builds = []
>       exit_status = 0
>  diff --git a/lib/python/bodhi_utils.py b/lib/python/bodhi_utils.py
>  index 94e1a7d..95d18a5 100644
>  --- 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.  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)?

The above would then change to ...

from autoqa.config import autoqa_cfg
server = autoqa_cfg.get('resources', 'bodhi_server', '')

>  +try:
>  +    fas_conf = SingleConfigParser()
>  +    fas_conf.read_single(['fas.conf', '/etc/autoqa/fas.conf']) 
Nice, fas.conf is definitely special and should only be used by
bodhi_utils so I don't see any problem with specifying the config_fail
paths as you have done here.  

>  +    fas = fas_conf.get_section('fas')
>  +    user = fas['username']
>  +    pswd = fas['password']
>  +    if not user or not pswd:
>  +        raise IOError
>  +except (IOError, ConfigParser.Error), e:
>  +    # if there's no fas.conf or it's incomplete, we just log in anonymously.
>  +    user = ''
>  +    pswd = ''
>  +
>  +if server:
>  +    bodhi = fedora.client.bodhi.BodhiClient(base_url=server, username=user,
> password=pswd)
>  +else:
>  +    bodhi = fedora.client.bodhi.BodhiClient(username=user, password=pswd)
>  +
>   def bodhitime(timestamp):
>       '''Convert timestamp (seconds since Epoch, assumed to be local time) to
> a
>       Bodhi-approved time string ('%Y-%m-%d %H:%M:%S')'''
>  @@ -40,7 +66,6 @@ def parse_bodhitime(bodhitimestr):
>   
>   def bodhi_list(params, limit=100):
>       '''Perform a bodhi 'list' method call, with pagination
> handling'''
>  -    bodhi = fedora.client.bodhi.BodhiClient()
>       params['tg_paginate_limit'] = limit
>       params['tg_paginate_no'] = 1
>   
>  @@ -136,7 +161,7 @@ def _is_bodhi_testresult_needed(old_result, comment_time, result,
> time_span):
>   
>       return True
>   
>  -def bodhi_post_testresult(update, testname, result, url, config, arch =
> 'noarch', karma = 0):
>  +def bodhi_post_testresult(update, testname, result, url, arch = 'noarch', karma
> = 0):
>       '''Post comment and karma to bodhi
>   
>          Args:
>  @@ -144,7 +169,6 @@ def bodhi_post_testresult(update, testname, result, url, config, arch
> = 'noarch'
>          testname -- the name of the test
>          result -- the result of the test
>          url -- url of the result of the test
>  -       config -- autoqa config
>          arch -- tested architecture (default 'noarch')
>          karma -- karma points (default 0)
>   
>  @@ -153,37 +177,25 @@ def bodhi_post_testresult(update, testname, result, url, config,
> arch = 'noarch'
>          posted (either posting is turned off or comment was already posted),
>          False otherwise.
>       '''
>  +
>  +    # TODO when new bodhi releases, add update identification by UPDATEID support
>  +
>       err_msg = 'Could not post a comment to bodhi'
>  -    conffiles = ['fas.conf', '/etc/autoqa/fas.conf']
>  -    try:
>  -        cfg_parser = SingleConfigParser()
>  -        cfg_parser.read_single(conffiles)
>  -        fas = cfg_parser.get_section('fas')
>  -    except (IOError, ConfigParser.Error), e:
>  -        print >> sys.stderr, "ERROR: Couldn't read none of config files:
> %s" % conffiles
>  -        print >> sys.stderr, e
>  -        return False
>   
>       if not update or not testname or not result or url == None:
>           sys.stderr.write('Incomplete arguments!\n%s\n' % err_msg)
>           return False
>   
>       try:
>  -        if not getbool(config.get('bodhi', 'send_bodhi_comments')):
>  -            print 'Sending bodhi comments is turned off. Test result will NOT be
> sent.'
>  -            return True
>  -    except KeyError:
>  +        if not getbool(autoqa_conf.get('notifications',
> 'send_bodhi_comments')):
>  +            raise ValueError
>  +    except ValueError:
>           print 'Sending bodhi comments is turned off. Test result will NOT be
> sent.'
>  -        # option missing -> it's false, do not send it (but return True since
>  +        # it's either False or not a bool -> it's false, do not send it (but
> return True since
>           # it's intentional, not an error)
>           return True
>   
>  -    try:
>  -        user = fas['username']
>  -        pswd = fas['password']
>  -        if not user or not pswd:
>  -            raise KeyError
>  -    except KeyError:
>  +    if not user or not pswd:
>           sys.stderr.write('Conf file containing FAS credentials is
> incomplete!\n%s\n' % err_msg)
>           return False
>   
>  @@ -191,14 +203,12 @@ def bodhi_post_testresult(update, testname, result, url, config,
> arch = 'noarch'
>                  % (testname, result, arch, url)
>       try:
>           (old_result, comment_time) = _bodhi_already_commented(update, user, testname,
> arch)
>  -        time_span = int(config.get('bodhi',
> 'bodhi_posting_comments_span'))
>  +        time_span = int(autoqa_conf.get('notifications',
> 'bodhi_posting_comments_span'))
>   
>           if not _is_bodhi_testresult_needed(old_result, comment_time, result,
> time_span):
>               print 'The test result already posted to bodhi.'
>               return True
>   
>  -        bodhi = fedora.client.BodhiClient(username=user, password=pswd)
>  -
>           if not bodhi.comment(update, comment, karma):
>               sys.stderr.write('%s\n') % err_msg
>               return False
>  @@ -216,10 +226,7 @@ def _self_test():
>       Simple self test.
>       '''
>       from datetime import timedelta
>  -    from ConfigParser import SafeConfigParser
>  -    cfg_parser = SafeConfigParser()
>  -    cfg_parser.read('/etc/autoqa/autoqa.conf')
>  -    time_span = int(cfg_parser.get('bodhi',
> 'bodhi_posting_comments_span'))
>  +    time_span = int(autoqa_conf.get('notifications',
> 'bodhi_posting_comments_span'))
>       try:
>           print '1. Test:',
>           assert _is_bodhi_testresult_needed('PASSED', datetime.now,
> 'PASSED', time_span) == False
>  diff --git a/lib/python/koji_utils.py b/lib/python/koji_utils.py
>  index 87b32e9..648412b 100644
>  --- 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?

>  +        self.server = cfg_parser.get('resources',
> 'koji_url')
>  +        self.pkgurl = cfg_parser.get('resources', 'pkg_url')
>  +        return koji.ClientSession.__init__(self, self.server, opts) 
See earlier comment on regarding command-line arguments and config
parsing...  I think we'll want to continue accepting the
'server=kojiurl' constructor argument so that callers can still specify
a different URL, and if not passed, we use the value specified in
autoqa.conf.  Once we have config file mgmt handled by autoqa/config.py,
I could see this change to ...

from autoqa.config import autoqa_cfg

kojiurl = autoqa_cfg.get('resources', 'koji_url',
'http://koji.fedoraproject.org')
pkgurl =  autoqa_cfg.get('resources', 'pkg_url',
'http://koji.fedoraproject.org/packages')

class SimpleKojiClientSession(koji.ClientSession):
   '''Convenience wrapper class for interacting with koji'''
   def __init__(self, server=kojiurl, opts=None):
       ...

>       def list_builds_since(self, timestamp):
>           '''Return a list of new builds since the given
> timestamp'''
>  @@ -147,7 +148,7 @@ class SimpleKojiClientSession(koji.ClientSession):
>   
>       def nvr_to_urls(self, nvr, arches=None, debuginfo=False):
>           info = self.getBuild(nvr)
>  -        baseurl = '/'.join((pkgurl, info['package_name'],
>  +        baseurl = '/'.join((self.pkgurl, info['package_name'],
>                              info['version'], info['release']))
>   
>           rpms = self.listRPMs(buildID=info['id'], arches=arches)
>  @@ -162,7 +163,7 @@ class SimpleKojiClientSession(koji.ClientSession):
>           key added with URL where to download the RPM as the value.'''
>   
>           info = self.getBuild(nvr)
>  -        baseurl = '/'.join((pkgurl, info['package_name'],
>  +        baseurl = '/'.join((self.pkgurl, info['package_name'],
>                              info['version'], info['release']))
>   
>           rpms = self.listRPMs(buildID=info['id'])
>  diff --git a/lib/python/test.py b/lib/python/test.py
>  index 29bb96c..73efc80 100644
>  --- a/lib/python/test.py
>  +++ b/lib/python/test.py
>  @@ -21,7 +21,7 @@ from autotest_lib.client.bin import test, utils
>   from autotest_lib.client.bin.test_config import config_loader
>   
>   from decorators import ExceptionCatcher
>  -from util import make_autotest_url
>  +from util import make_autotest_url, getbool
>   import os, sys, traceback
>   
>   class AutoQATest(test.test, object):
>  @@ -80,7 +80,7 @@ class AutoQATest(test.test, object):
>   
>           # prepare email
>           send_to = []
>  -        result_email = self.config.get('test','result_email')
>  +        result_email = self.config.get('notifications','result_email')
>           if result_email and isinstance(result_email, str):
>               send_to = result_email.split(',')
>               try:
>  @@ -114,12 +114,12 @@ class AutoQATest(test.test, object):
>           log.close()
>   
>           # send email
>  -        if send_to:
>  +        if getbool(self.config.get('notifications',
> 'send_result_email')) and send_to:
>               utils.send_email(mail_to=send_to,
>  -                    mail_from=self.config.get('test','mail_from'),
>  -                    mail_server=self.config.get('test','smtpserver'),
>  -                    subject=self.summary,
>  -                    body = mail_body)
>  +                mail_from=self.config.get('notifications','mail_from'),
>  +               
> mail_server=self.config.get('notifications','smtpserver'),
>  +                subject=self.summary,
>  +                body = mail_body)
>   
>       def _convert_list_variables(self):
>           '''
>  diff --git a/tests/initscripts/initscripts.py b/tests/initscripts/initscripts.py
>  index 8e35e35..097fbc8 100644
>  --- a/tests/initscripts/initscripts.py
>  +++ b/tests/initscripts/initscripts.py
>  @@ -43,7 +43,7 @@ class initscripts(AutoQATest):
>               f = open("/etc/yum.repos.d/beakerlib.repo", "w")
>               f.write("[beakerlib-testing]\nname=Testing repo for
> BeakerLib\nbaseurl=http://afri.fedorapeople.org/beakerlib/\nenabled=1\ngp...)
>               f.close()
>  -        utils.system('yum -y install beakerlib') 
>  +        utils.system('yum -y install beakerlib')
>   
>       @ExceptionCatcher()
>       def initialize(self, config, **kwargs):
>  @@ -174,7 +174,9 @@ class initscripts(AutoQATest):
>   
>               # email results to mailing list and to pkg owner if they optin
>               repo = repoinfo.getrepo_by_tag(kojitag)
>  -            if repo is not None and autoqa.util.check_opt_in(name,
> repo['collection_name']):
>  +            send_optin_email = getbool(self.config.get('notifications',
> 'send_optin_email'))
>  +            if repo is not None and send_optin_email and \
>  +            autoqa.util.check_opt_in(pkg_name, repo['collection_name']):
>                   #FIXME - hardcoded partial email address here - obviously sub-par
>                   self.mail_to.append('%s-owner(a)fedoraproject.org' % name)
>   
>  diff --git a/tests/rpmguard/rpmguard.py b/tests/rpmguard/rpmguard.py
>  index bf973f0..4a20d53 100644
>  --- a/tests/rpmguard/rpmguard.py
>  +++ b/tests/rpmguard/rpmguard.py
>  @@ -82,7 +82,9 @@ class rpmguard(AutoQATest):
>               # email results to mailing list and to pkg owner if they optin
>               repo = repoinfo.getrepo_by_tag(kojitag)
>               pkg_name = rpmUtils.miscutils.splitFilename(envr + '.noarch')[0]
>  -            if repo is not None and autoqa.util.check_opt_in(pkg_name,
> repo['collection_name']):
>  +            send_optin_email = getbool(self.config.get('notifications',
> 'send_optin_email'))
>  +            if repo is not None and send_optin_email and \
>  +            autoqa.util.check_opt_in(pkg_name, repo['collection_name']):
>                   #FIXME - hardcoded partial email address here - obviously sub-par
>                   self.mail_to.append('%s-owner(a)fedoraproject.org' % pkg_name)
>   
>  diff --git a/tests/rpmlint/rpmlint.py b/tests/rpmlint/rpmlint.py
>  index 1c06a8b..cac8a48 100644
>  --- a/tests/rpmlint/rpmlint.py
>  +++ b/tests/rpmlint/rpmlint.py
>  @@ -116,7 +116,9 @@ class rpmlint(AutoQATest):
>               # email results to mailing list and to pkg owner if they optin
>               repo = repoinfo.getrepo_by_tag(kojitag)
>               pkg_name = rpmUtils.miscutils.splitFilename(envr + '.noarch')[0]
>  -            if repo is not None and autoqa.util.check_opt_in(pkg_name,
> repo['collection_name']):
>  +            send_optin_email = getbool(self.config.get('notifications',
> 'send_optin_email')) 
Do we need to import the getbool method, or are we getting that for free
due to autoqa.test being imported already?

>  +            if repo is not None and send_optin_email and \
>  +            autoqa.util.check_opt_in(pkg_name, repo['collection_name']):
>                   #FIXME - hardcoded partial email address here - obviously sub-par
>                   self.mail_to.append('%s-owner(a)fedoraproject.org' % pkg_name)
>    
Thanks,
James

[Martin Krizek]

You can find the updated patch on mkrizek-staging branch (to see diff from last patch,
just do diff between mkrizek-staging-ready and mkrizek-staging), more
straight-to-the-point comments on changes below:

----- Original Message -----
>  From: "James Laska" <jlaska(a)redhat.com&gt;
>  To: "AutoQA development" <autoqa-devel(a)lists.fedorahosted.org&gt;
>  Sent: Wednesday, December 15, 2010 5:05:23 PM
>  Subject: Re: [PATCH] Add support for a staging server.
>  On Wed, 2010-12-15 at 08:25 -0500, Martin Krizek wrote:
>  > Hey all,
>  >
>  > this patch solves ticket #241. It adds options in autoqa.conf to
>  > turn on/off
>  > optin, notifications and results e-mails. There are options
>  > containing urls of
>  > koji and bodhi instances that will be used.
>  >
>  > Also there are some alterations in bodhi_utils, there is only one
>  > creation of
>  > BodhiClient instance and one config files reading to simplify the
>  > code.
>  >
>  > You can find the patch in mkrizek-staging-ready branch.
>  
>  Thanks Martin!
>  
>  I'm a bit wordy in my comments, but the general theme around avoiding
>  potentially duplicate autoqa.conf options and figuring out a
>  consistent
>  approach for how we want to handle cli arguments, load autoqa.conf
>  defaults and where's the best place for that code to live.
>  
>  > ---
>  > 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?
>   
Sounds reasonable, send_notification_email was removed.

>  >      'autotest_server': socket.gethostname(),
>  >  }
>  >
>  >  cfg_parser = SingleConfigParser()
>  >  cfg_parser.read(cfgfile)
>  > -conf = cfg_parser.get_section('general', conf) # used by
>  > prep_controlfile
>  > +conf = cfg_parser.get_section('general', conf)
>  > +conf = cfg_parser.get_section('notifications', conf)
>  >  # we don't need to catch errors here, bcz we want autoqa to crash
>  >  for invalid config
>  >
>  >  def prep_controlfile(controlfile, extradata):
>  > @@ -283,7 +285,10 @@ for test in testlist:
>  >
>  >      for arch in test_vars[test]['archs']:
>  >          testname='%s:%s.%s' % (hookname, test, arch)
>  > - email = conf['notification_email']
>  > + if conf['send_notification_email']:
>  > + email = conf['notification_email']
>  > + else:
>  > + email = ''
>  
>  This depends on whether we use multiple configuration options to
>  enable
>  email notification. If we continue with the single option, I'm fond of
>  adding a default value when loading a value from a dictionary ...
>  
>  email = conf.get('notification_mail', '')
>   
Done.

>  >          if run_local:
>  >              retval = run_test_locally(control, name=testname,
>  > diff --git a/autoqa.conf b/autoqa.conf
>  > index 69278fe..886536e 100644
>  > --- a/autoqa.conf
>  > +++ b/autoqa.conf
>  > @@ -6,29 +6,62 @@
>  >  hookdir = /usr/share/autoqa
>  >  # Path containing the actual autoqa tests to be run
>  >  testdir = /usr/share/autotest/client/site_tests
>  > -# comma-separated email addresses that will get notification of
>  > test completion
>  > -# (leave blank to disable)
>  > -notification_email =
>  >  # Set to "true" if you want autoqa to always run tests locally
>  >  using 'autotest'.
>  >  # Default is to schedule jobs with the autotest server using
>  >  'atest'.
>  >  # This is the same as using 'autoqa --local'.
>  > -#local = true
>  > +local = false
>  > +# Hostname of this autotest server. Used for constructing URLs for
>  > accessing
>  > +# test results. By default current hostname is used.
>  > +autotest_server =
>  >
>  > -# The 'test' section is used for systemwide configuration of autoqa
>  > tests
>  > -[test]
>  > -# comma-separated list of email addresses that will receive full
>  > test results
>  > -# (leave blank to disable)
>  > -result_email =
>  > -# email address for From: line of emails sent by tests
>  > -mail_from = autoqa(a)fedoraproject.org
>  > -# hostname or hostname:port of smtp server / mailhub to use for
>  > sending email
>  > +# The 'notifications' section controls which notifications about
>  > which events
>  > +# you want to send
>  > +[notifications]
>  > +# Sender email address used for all emails sent from AutoQA.
>  > +mail_from =
>  > +# Hostname or hostname:port of SMTP server used for sending emails.
>  > +# Please note that the emails are sent from the autotest clients,
>  > not the
>  > +# server.
>  >  smtpserver = localhost
>  >
>  > -[bodhi]
>  > -# If "true", test results (for tests utilizing this feature) will
>  > be sent
>  > -# as comments to Fedora Update System (Bodhi). This requires that
>  > you have
>  > -# Bodhi credentials filled in in fas.conf.
>  > +# Comma-separated list of email addresses that will receive full
>  > test results
>  > +# of every completed test.
>  > +result_email =
>  > +# If "true", result email will be sent. Otherwise "false".
>  > +send_result_email = false
>  > +
>  > +# Opt-in emails are sent (for selected tests) to the maintainers of
>  > a package
>  > +# that has just been tested and the maintainer opted in. Read more
>  > at:
>  > +#
>  >
> http://jlaska.wordpress.com/2010/06/01/fedora-package-maintainers-want-te...
>  > +# If "true", opt-in email will be sent. Otherwise "false".
>  > +send_optin_email = false
>  > +
>  > +# Comma-separated email addresses that will get notification of
>  > every job
>  > +# completion or every job state change as defined in
>  > 'notify_email_statuses'
>  > +# in ~autotest/global_config.ini.
>  > +# Important: These emails are handled and sent by autotest server,
>  > the
>  > +# 'mail_from' and 'smtpserver' options don't apply for it.
>  > +# Read more at http://autotest.kernel.org/wiki/GlobalConfig (search
>  > for
>  > +# 'notify_email').
>  > +notification_email =
>  > +# If "true", notification email will be sent. Otherwise
> "false".
>  > +send_notification_email = false
>  
>  Same as earlier, it seems weird to have two switches to enable/disable
>  email notification.
>   
Same, removed.

>  > +# If "true", test results (for selected tests) will
> be sent as
>  > comments to
>  > +# Fedora Update System (Bodhi). This requires that you have Bodhi
>  > credentials
>  > +# filled in in fas.conf.
>  >  send_bodhi_comments = false
>  > -# how long (minutes) should we wait before posting the same comment
>  > to bodhi
>  > -# by default 3 days (3*24*60 = 4320)
>  > +# How long (in minutes) should we wait before allowing consequent
>  > test to
>  > +# re-post a 'FAILED' comment into Bodhi once again.
>  > +# By default 3 days (3*24*60 = 4320).
>  >  bodhi_posting_comments_span = 4320
>  >
>  > +# 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?
>  
>  > 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?
>   
Yes, the -s/--server option was added to koji watcher.

>  >  def get_prevtime():
>  >      '''Returns the prevtime to use when looking for new builds.
>  >      Value will be
>  > @@ -126,7 +123,7 @@ tags for new builds and kick off tests when new
>  > builds/packages are found.')
>  >
>  >      # Set up the koji connection
>  >      kojiopts = {} # Possible items: user, password, debug_xmlrpc,
>  >      debug..
>  > - session = koji_utils.SimpleKojiClientSession(kojiserver, kojiopts)
>  > + session = koji_utils.SimpleKojiClientSession(kojiopts)
>  >      untagged_builds = []
>  >      tagged_builds = []
>  >      exit_status = 0
>  > diff --git a/lib/python/bodhi_utils.py b/lib/python/bodhi_utils.py
>  > index 94e1a7d..95d18a5 100644
>  > --- 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. 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)?
>  
>  The above would then change to ...
>  
>  from autoqa.config import autoqa_cfg
>  server = autoqa_cfg.get('resources', 'bodhi_server', '')
>   
I like the idea, done.

>  > +try:
>  > + fas_conf = SingleConfigParser()
>  > + fas_conf.read_single(['fas.conf', '/etc/autoqa/fas.conf'])
>  
>  Nice, fas.conf is definitely special and should only be used by
>  bodhi_utils so I don't see any problem with specifying the config_fail
>  paths as you have done here.
>  
>  > + fas = fas_conf.get_section('fas')
>  > + user = fas['username']
>  > + pswd = fas['password']
>  > + if not user or not pswd:
>  > + raise IOError
>  > +except (IOError, ConfigParser.Error), e:
>  > + # if there's no fas.conf or it's incomplete, we just log in
>  > anonymously.
>  > + user = ''
>  > + pswd = ''
>  > +
>  > +if server:
>  > + bodhi = fedora.client.bodhi.BodhiClient(base_url=server,
>  > username=user, password=pswd)
>  > +else:
>  > + bodhi = fedora.client.bodhi.BodhiClient(username=user,
>  > password=pswd)
>  > +
>  >  def bodhitime(timestamp):
>  >      '''Convert timestamp (seconds since Epoch, assumed to be local
>  >      time) to a
>  >      Bodhi-approved time string ('%Y-%m-%d %H:%M:%S')'''
>  > @@ -40,7 +66,6 @@ def parse_bodhitime(bodhitimestr):
>  >
>  >  def bodhi_list(params, limit=100):
>  >      '''Perform a bodhi 'list' method call, with pagination
>  >      handling'''
>  > - bodhi = fedora.client.bodhi.BodhiClient()
>  >      params['tg_paginate_limit'] = limit
>  >      params['tg_paginate_no'] = 1
>  >
>  > @@ -136,7 +161,7 @@ def _is_bodhi_testresult_needed(old_result,
>  > comment_time, result, time_span):
>  >
>  >      return True
>  >
>  > -def bodhi_post_testresult(update, testname, result, url, config,
>  > arch = 'noarch', karma = 0):
>  > +def bodhi_post_testresult(update, testname, result, url, arch =
>  > 'noarch', karma = 0):
>  >      '''Post comment and karma to bodhi
>  >
>  >         Args:
>  > @@ -144,7 +169,6 @@ def bodhi_post_testresult(update, testname,
>  > result, url, config, arch = 'noarch'
>  >         testname -- the name of the test
>  >         result -- the result of the test
>  >         url -- url of the result of the test
>  > - config -- autoqa config
>  >         arch -- tested architecture (default 'noarch')
>  >         karma -- karma points (default 0)
>  >
>  > @@ -153,37 +177,25 @@ def bodhi_post_testresult(update, testname,
>  > result, url, config, arch = 'noarch'
>  >         posted (either posting is turned off or comment was already
>  >         posted),
>  >         False otherwise.
>  >      '''
>  > +
>  > + # TODO when new bodhi releases, add update identification by
>  > UPDATEID support
>  > +
>  >      err_msg = 'Could not post a comment to bodhi'
>  > - conffiles = ['fas.conf', '/etc/autoqa/fas.conf']
>  > - try:
>  > - cfg_parser = SingleConfigParser()
>  > - cfg_parser.read_single(conffiles)
>  > - fas = cfg_parser.get_section('fas')
>  > - except (IOError, ConfigParser.Error), e:
>  > - print >> sys.stderr, "ERROR: Couldn't read none of config files:
>  > %s" % conffiles
>  > - print >> sys.stderr, e
>  > - return False
>  >
>  >      if not update or not testname or not result or url == None:
>  >          sys.stderr.write('Incomplete arguments!\n%s\n' % err_msg)
>  >          return False
>  >
>  >      try:
>  > - if not getbool(config.get('bodhi', 'send_bodhi_comments')):
>  > - print 'Sending bodhi comments is turned off. Test result will NOT
>  > be sent.'
>  > - return True
>  > - except KeyError:
>  > + if not getbool(autoqa_conf.get('notifications',
>  > 'send_bodhi_comments')):
>  > + raise ValueError
>  > + except ValueError:
>  >          print 'Sending bodhi comments is turned off. Test result
>  >          will NOT be sent.'
>  > - # option missing -> it's false, do not send it (but return True
>  > since
>  > + # it's either False or not a bool -> it's false, do not send it
>  > (but return True since
>  >          # it's intentional, not an error)
>  >          return True
>  >
>  > - try:
>  > - user = fas['username']
>  > - pswd = fas['password']
>  > - if not user or not pswd:
>  > - raise KeyError
>  > - except KeyError:
>  > + if not user or not pswd:
>  >          sys.stderr.write('Conf file containing FAS credentials is
>  >          incomplete!\n%s\n' % err_msg)
>  >          return False
>  >
>  > @@ -191,14 +203,12 @@ def bodhi_post_testresult(update, testname,
>  > result, url, config, arch = 'noarch'
>  >                 % (testname, result, arch, url)
>  >      try:
>  >          (old_result, comment_time) =
>  >          _bodhi_already_commented(update, user, testname, arch)
>  > - time_span = int(config.get('bodhi',
>  > 'bodhi_posting_comments_span'))
>  > + time_span = int(autoqa_conf.get('notifications',
>  > 'bodhi_posting_comments_span'))
>  >
>  >          if not _is_bodhi_testresult_needed(old_result,
>  >          comment_time, result, time_span):
>  >              print 'The test result already posted to bodhi.'
>  >              return True
>  >
>  > - bodhi = fedora.client.BodhiClient(username=user, password=pswd)
>  > -
>  >          if not bodhi.comment(update, comment, karma):
>  >              sys.stderr.write('%s\n') % err_msg
>  >              return False
>  > @@ -216,10 +226,7 @@ def _self_test():
>  >      Simple self test.
>  >      '''
>  >      from datetime import timedelta
>  > - from ConfigParser import SafeConfigParser
>  > - cfg_parser = SafeConfigParser()
>  > - cfg_parser.read('/etc/autoqa/autoqa.conf')
>  > - time_span = int(cfg_parser.get('bodhi',
>  > 'bodhi_posting_comments_span'))
>  > + time_span = int(autoqa_conf.get('notifications',
>  > 'bodhi_posting_comments_span'))
>  >      try:
>  >          print '1. Test:',
>  >          assert _is_bodhi_testresult_needed('PASSED', datetime.now,
>  >          'PASSED', time_span) == False
>  > diff --git a/lib/python/koji_utils.py b/lib/python/koji_utils.py
>  > index 87b32e9..648412b 100644
>  > --- 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?
>  
>  > + self.server = cfg_parser.get('resources', 'koji_url')
>  > + self.pkgurl = cfg_parser.get('resources', 'pkg_url')
>  > + return koji.ClientSession.__init__(self, self.server, opts)
>  
>  See earlier comment on regarding command-line arguments and config
>  parsing... I think we'll want to continue accepting the
>  'server=kojiurl' constructor argument so that callers can still
>  specify
>  a different URL, and if not passed, we use the value specified in
>  autoqa.conf. Once we have config file mgmt handled by
>  autoqa/config.py,
>  I could see this change to ...
>  
>  from autoqa.config import autoqa_cfg
>  
>  kojiurl = autoqa_cfg.get('resources', 'koji_url',
>  'http://koji.fedoraproject.org')
>  pkgurl = autoqa_cfg.get('resources', 'pkg_url',
>  'http://koji.fedoraproject.org/packages')
>  
>  class SimpleKojiClientSession(koji.ClientSession):
>  '''Convenience wrapper class for interacting with koji'''
>  def __init__(self, server=kojiurl, opts=None):
>  ...
>  
>  >      def list_builds_since(self, timestamp):
>  >          '''Return a list of new builds since the given
> timestamp'''
>  > @@ -147,7 +148,7 @@ class
>  > SimpleKojiClientSession(koji.ClientSession):
>  >
>  >      def nvr_to_urls(self, nvr, arches=None, debuginfo=False):
>  >          info = self.getBuild(nvr)
>  > - baseurl = '/'.join((pkgurl, info['package_name'],
>  > + baseurl = '/'.join((self.pkgurl, info['package_name'],
>  >                             info['version'], info['release']))
>  >
>  >          rpms = self.listRPMs(buildID=info['id'], arches=arches)
>  > @@ -162,7 +163,7 @@ class
>  > SimpleKojiClientSession(koji.ClientSession):
>  >          key added with URL where to download the RPM as the
>  >          value.'''
>  >
>  >          info = self.getBuild(nvr)
>  > - baseurl = '/'.join((pkgurl, info['package_name'],
>  > + baseurl = '/'.join((self.pkgurl, info['package_name'],
>  >                             info['version'], info['release']))
>  >
>  >          rpms = self.listRPMs(buildID=info['id'])
>  > diff --git a/lib/python/test.py b/lib/python/test.py
>  > index 29bb96c..73efc80 100644
>  > --- a/lib/python/test.py
>  > +++ b/lib/python/test.py
>  > @@ -21,7 +21,7 @@ from autotest_lib.client.bin import test, utils
>  >  from autotest_lib.client.bin.test_config import config_loader
>  >
>  >  from decorators import ExceptionCatcher
>  > -from util import make_autotest_url
>  > +from util import make_autotest_url, getbool
>  >  import os, sys, traceback
>  >
>  >  class AutoQATest(test.test, object):
>  > @@ -80,7 +80,7 @@ class AutoQATest(test.test, object):
>  >
>  >          # prepare email
>  >          send_to = []
>  > - result_email = self.config.get('test','result_email')
>  > + result_email = self.config.get('notifications','result_email')
>  >          if result_email and isinstance(result_email, str):
>  >              send_to = result_email.split(',')
>  >              try:
>  > @@ -114,12 +114,12 @@ class AutoQATest(test.test, object):
>  >          log.close()
>  >
>  >          # send email
>  > - if send_to:
>  > + if getbool(self.config.get('notifications', 'send_result_email'))
>  > and send_to:
>  >              utils.send_email(mail_to=send_to,
>  > - mail_from=self.config.get('test','mail_from'),
>  > - mail_server=self.config.get('test','smtpserver'),
>  > - subject=self.summary,
>  > - body = mail_body)
>  > + mail_from=self.config.get('notifications','mail_from'),
>  > + mail_server=self.config.get('notifications','smtpserver'),
>  > + subject=self.summary,
>  > + body = mail_body)
>  >
>  >      def _convert_list_variables(self):
>  >          '''
>  > diff --git a/tests/initscripts/initscripts.py
>  > b/tests/initscripts/initscripts.py
>  > index 8e35e35..097fbc8 100644
>  > --- a/tests/initscripts/initscripts.py
>  > +++ b/tests/initscripts/initscripts.py
>  > @@ -43,7 +43,7 @@ class initscripts(AutoQATest):
>  >              f = open("/etc/yum.repos.d/beakerlib.repo", "w")
>  >              f.write("[beakerlib-testing]\nname=Testing repo for
>  >             
> BeakerLib\nbaseurl=http://afri.fedorapeople.org/beakerlib/\nenabled=1\ngp...)
>  >              f.close()
>  > - utils.system('yum -y install beakerlib')
>  > + utils.system('yum -y install beakerlib')
>  >
>  >      @ExceptionCatcher()
>  >      def initialize(self, config, **kwargs):
>  > @@ -174,7 +174,9 @@ class initscripts(AutoQATest):
>  >
>  >              # email results to mailing list and to pkg owner if
>  >              they optin
>  >              repo = repoinfo.getrepo_by_tag(kojitag)
>  > - if repo is not None and autoqa.util.check_opt_in(name,
>  > repo['collection_name']):
>  > + send_optin_email = getbool(self.config.get('notifications',
>  > 'send_optin_email'))
>  > + if repo is not None and send_optin_email and \
>  > + autoqa.util.check_opt_in(pkg_name, repo['collection_name']):
>  >                  #FIXME - hardcoded partial email address here -
>  >                  obviously sub-par
>  >                  self.mail_to.append(&#39;%s-owner(a)fedoraproject.org&#39; %
>  >                  name)
>  >
>  > diff --git a/tests/rpmguard/rpmguard.py b/tests/rpmguard/rpmguard.py
>  > index bf973f0..4a20d53 100644
>  > --- a/tests/rpmguard/rpmguard.py
>  > +++ b/tests/rpmguard/rpmguard.py
>  > @@ -82,7 +82,9 @@ class rpmguard(AutoQATest):
>  >              # email results to mailing list and to pkg owner if
>  >              they optin
>  >              repo = repoinfo.getrepo_by_tag(kojitag)
>  >              pkg_name = rpmUtils.miscutils.splitFilename(envr +
>  >              '.noarch')[0]
>  > - if repo is not None and autoqa.util.check_opt_in(pkg_name,
>  > repo['collection_name']):
>  > + send_optin_email = getbool(self.config.get('notifications',
>  > 'send_optin_email'))
>  > + if repo is not None and send_optin_email and \
>  > + autoqa.util.check_opt_in(pkg_name, repo['collection_name']):
>  >                  #FIXME - hardcoded partial email address here -
>  >                  obviously sub-par
>  >                  self.mail_to.append(&#39;%s-owner(a)fedoraproject.org&#39; %
>  >                  pkg_name)
>  >
>  > diff --git a/tests/rpmlint/rpmlint.py b/tests/rpmlint/rpmlint.py
>  > index 1c06a8b..cac8a48 100644
>  > --- a/tests/rpmlint/rpmlint.py
>  > +++ b/tests/rpmlint/rpmlint.py
>  > @@ -116,7 +116,9 @@ class rpmlint(AutoQATest):
>  >              # email results to mailing list and to pkg owner if
>  >              they optin
>  >              repo = repoinfo.getrepo_by_tag(kojitag)
>  >              pkg_name = rpmUtils.miscutils.splitFilename(envr +
>  >              '.noarch')[0]
>  > - if repo is not None and autoqa.util.check_opt_in(pkg_name,
>  > repo['collection_name']):
>  > + send_optin_email = getbool(self.config.get('notifications',
>  > 'send_optin_email'))
>  
>  Do we need to import the getbool method, or are we getting that for
>  free
>  due to autoqa.test being imported already?
>  
>  > + if repo is not None and send_optin_email and \
>  > + autoqa.util.check_opt_in(pkg_name, repo['collection_name']):
>  >                  #FIXME - hardcoded partial email address here -
>  >                  obviously sub-par
>  >                  self.mail_to.append(&#39;%s-owner(a)fedoraproject.org&#39; %
>  >                  pkg_name)
>  >
>  
>  Thanks,
>  James
>  
>  _______________________________________________
>  autoqa-devel mailing list
>  autoqa-devel(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/autoqa-devel

[James Laska]

On Tue, 2010-12-21 at 11:32 -0500, Martin Krizek wrote:
>  You can find the updated patch on mkrizek-staging branch (to see
> diff
>  from last patch, just do diff between mkrizek-staging-ready and
>  mkrizek-staging), more straight-to-the-point comments on changes
>  below: 
Looks good, nicely done!  ACK from me.

>  ----- Original Message -----
>  > From: "James Laska" <jlaska(a)redhat.com&gt;
>  > To: "AutoQA development" <autoqa-devel(a)lists.fedorahosted.org&gt;
>  > Sent: Wednesday, December 15, 2010 5:05:23 PM
>  > Subject: Re: [PATCH] Add support for a staging server.
>  > On Wed, 2010-12-15 at 08:25 -0500, Martin Krizek wrote:
>  > > Hey all,
>  > >
>  > > this patch solves ticket #241. It adds options in autoqa.conf to
>  > > turn on/off
>  > > optin, notifications and results e-mails. There are options
>  > > containing urls of
>  > > koji and bodhi instances that will be used.
>  > >
>  > > Also there are some alterations in bodhi_utils, there is only one
>  > > creation of
>  > > BodhiClient instance and one config files reading to simplify the
>  > > code.
>  > >
>  > > You can find the patch in mkrizek-staging-ready branch.
>  > 
>  > Thanks Martin!
>  > 
>  > I'm a bit wordy in my comments, but the general theme around avoiding
>  > potentially duplicate autoqa.conf options and figuring out a
>  > consistent
>  > approach for how we want to handle cli arguments, load autoqa.conf
>  > defaults and where's the best place for that code to live.
>  > 
>  > > ---
>  > > 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?
>  > 
>  
>  Sounds reasonable, send_notification_email was removed.
>  
>  > >      'autotest_server': socket.gethostname(),
>  > >  }
>  > >
>  > >  cfg_parser = SingleConfigParser()
>  > >  cfg_parser.read(cfgfile)
>  > > -conf = cfg_parser.get_section('general', conf) # used by
>  > > prep_controlfile
>  > > +conf = cfg_parser.get_section('general', conf)
>  > > +conf = cfg_parser.get_section('notifications', conf)
>  > >  # we don't need to catch errors here, bcz we want autoqa to crash
>  > >  for invalid config
>  > >
>  > >  def prep_controlfile(controlfile, extradata):
>  > > @@ -283,7 +285,10 @@ for test in testlist:
>  > >
>  > >      for arch in test_vars[test]['archs']:
>  > >          testname='%s:%s.%s' % (hookname, test, arch)
>  > > - email = conf['notification_email']
>  > > + if conf['send_notification_email']:
>  > > + email = conf['notification_email']
>  > > + else:
>  > > + email = ''
>  > 
>  > This depends on whether we use multiple configuration options to
>  > enable
>  > email notification. If we continue with the single option, I'm fond of
>  > adding a default value when loading a value from a dictionary ...
>  > 
>  > email = conf.get('notification_mail', '')
>  > 
>  
>  Done.
>  
>  > >          if run_local:
>  > >              retval = run_test_locally(control, name=testname,
>  > > diff --git a/autoqa.conf b/autoqa.conf
>  > > index 69278fe..886536e 100644
>  > > --- a/autoqa.conf
>  > > +++ b/autoqa.conf
>  > > @@ -6,29 +6,62 @@
>  > >  hookdir = /usr/share/autoqa
>  > >  # Path containing the actual autoqa tests to be run
>  > >  testdir = /usr/share/autotest/client/site_tests
>  > > -# comma-separated email addresses that will get notification of
>  > > test completion
>  > > -# (leave blank to disable)
>  > > -notification_email =
>  > >  # Set to "true" if you want autoqa to always run tests locally
>  > >  using 'autotest'.
>  > >  # Default is to schedule jobs with the autotest server using
>  > >  'atest'.
>  > >  # This is the same as using 'autoqa --local'.
>  > > -#local = true
>  > > +local = false
>  > > +# Hostname of this autotest server. Used for constructing URLs for
>  > > accessing
>  > > +# test results. By default current hostname is used.
>  > > +autotest_server =
>  > >
>  > > -# The 'test' section is used for systemwide configuration of autoqa
>  > > tests
>  > > -[test]
>  > > -# comma-separated list of email addresses that will receive full
>  > > test results
>  > > -# (leave blank to disable)
>  > > -result_email =
>  > > -# email address for From: line of emails sent by tests
>  > > -mail_from = autoqa(a)fedoraproject.org
>  > > -# hostname or hostname:port of smtp server / mailhub to use for
>  > > sending email
>  > > +# The 'notifications' section controls which notifications about
>  > > which events
>  > > +# you want to send
>  > > +[notifications]
>  > > +# Sender email address used for all emails sent from AutoQA.
>  > > +mail_from =
>  > > +# Hostname or hostname:port of SMTP server used for sending emails.
>  > > +# Please note that the emails are sent from the autotest clients,
>  > > not the
>  > > +# server.
>  > >  smtpserver = localhost
>  > >
>  > > -[bodhi]
>  > > -# If "true", test results (for tests utilizing this feature) will
>  > > be sent
>  > > -# as comments to Fedora Update System (Bodhi). This requires that
>  > > you have
>  > > -# Bodhi credentials filled in in fas.conf.
>  > > +# Comma-separated list of email addresses that will receive full
>  > > test results
>  > > +# of every completed test.
>  > > +result_email =
>  > > +# If "true", result email will be sent. Otherwise
> "false".
>  > > +send_result_email = false
>  > > +
>  > > +# Opt-in emails are sent (for selected tests) to the maintainers of
>  > > a package
>  > > +# that has just been tested and the maintainer opted in. Read more
>  > > at:
>  > > +#
>  > >
> http://jlaska.wordpress.com/2010/06/01/fedora-package-maintainers-want-te...
>  > > +# If "true", opt-in email will be sent. Otherwise
> "false".
>  > > +send_optin_email = false
>  > > +
>  > > +# Comma-separated email addresses that will get notification of
>  > > every job
>  > > +# completion or every job state change as defined in
>  > > 'notify_email_statuses'
>  > > +# in ~autotest/global_config.ini.
>  > > +# Important: These emails are handled and sent by autotest server,
>  > > the
>  > > +# 'mail_from' and 'smtpserver' options don't apply for
> it.
>  > > +# Read more at http://autotest.kernel.org/wiki/GlobalConfig (search
>  > > for
>  > > +# 'notify_email').
>  > > +notification_email =
>  > > +# If "true", notification email will be sent. Otherwise
> "false".
>  > > +send_notification_email = false
>  > 
>  > Same as earlier, it seems weird to have two switches to enable/disable
>  > email notification.
>  > 
>  
>  Same, removed.
>  
>  > > +# If "true", test results (for selected tests) will be sent as
>  > > comments to
>  > > +# Fedora Update System (Bodhi). This requires that you have Bodhi
>  > > credentials
>  > > +# filled in in fas.conf.
>  > >  send_bodhi_comments = false
>  > > -# how long (minutes) should we wait before posting the same comment
>  > > to bodhi
>  > > -# by default 3 days (3*24*60 = 4320)
>  > > +# How long (in minutes) should we wait before allowing consequent
>  > > test to
>  > > +# re-post a 'FAILED' comment into Bodhi once again.
>  > > +# By default 3 days (3*24*60 = 4320).
>  > >  bodhi_posting_comments_span = 4320
>  > >
>  > > +# 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?
>  > 
>  > > 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?
>  > 
>  
>  Yes, the -s/--server option was added to koji watcher.
>  
>  > >  def get_prevtime():
>  > >      '''Returns the prevtime to use when looking for new builds.
>  > >      Value will be
>  > > @@ -126,7 +123,7 @@ tags for new builds and kick off tests when new
>  > > builds/packages are found.')
>  > >
>  > >      # Set up the koji connection
>  > >      kojiopts = {} # Possible items: user, password, debug_xmlrpc,
>  > >      debug..
>  > > - session = koji_utils.SimpleKojiClientSession(kojiserver, kojiopts)
>  > > + session = koji_utils.SimpleKojiClientSession(kojiopts)
>  > >      untagged_builds = []
>  > >      tagged_builds = []
>  > >      exit_status = 0
>  > > diff --git a/lib/python/bodhi_utils.py b/lib/python/bodhi_utils.py
>  > > index 94e1a7d..95d18a5 100644
>  > > --- 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. 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)?
>  > 
>  > The above would then change to ...
>  > 
>  > from autoqa.config import autoqa_cfg
>  > server = autoqa_cfg.get('resources', 'bodhi_server', '')
>  > 
>  
>  I like the idea, done. 
Cool, glad that was helpful!  I wasn't sure if they fell in the scope of
your patchset, but nice to see that included either way.

Thanks,
James

[Kamil Paral]

----- Original Message -----
>  On Tue, 2010-12-21 at 11:32 -0500, Martin Krizek wrote:
>  > You can find the updated patch on mkrizek-staging branch (to see
>  > diff
>  > from last patch, just do diff between mkrizek-staging-ready and
>  > mkrizek-staging), more straight-to-the-point comments on changes
>  > below:
>  
>  Looks good, nicely done! ACK from me. 
I have added a few one-line changes [1] and then committed to
master [2].

Thanks, Martin!

[1]
http://git.fedorahosted.org/git/?p=autoqa.git;a=commitdiff;h=6a9f55a1c503...
[2]
http://git.fedorahosted.org/git/?p=autoqa.git;a=commitdiff;h=5c144110b64e...