Re: [PATCH] Add support for a staging server.

[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