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@redhat.com To: "AutoQA development" autoqa-devel@lists.fedorahosted.org 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
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@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-test-... +# 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 ...
- 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