[PATCH 1/2] Move config processing from CLI to koji.read_config().
Mike McLean
mikem at redhat.com
Fri Oct 30 01:18:56 UTC 2015
On 08/27/2015 09:15 AM, Daniel Mach wrote:
> ---
> cli/koji | 85 ++------------------------------------------
> koji/__init__.py | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 109 insertions(+), 82 deletions(-)
I finally got a chance to give this a proper review and I'm afraid it
still needs more work....
There is an issue with the way get_profile_module works. By copying the
contents of the original module into the duplicate module, they retain
their connection to the original module globals. So when newmod.PathInfo
refers to BASEDIR, it is getting koji.BASEDIR not newmod.BASEDIR, which
leads to incorrect and surprising behavior.
That part at least is easy enough to fix. Some of the other things will
be more work.
The patch performs
config = read_config("koji")
in the main koji module. I really disagree with automatically loading
default configs at module load time. The executing code should be given
the chance to control this. As near as I can tell, nothing relies on the
koji module itself having this global defined, so I think we can just
remove this.
_config_directory_contents appears to be an exact duplicate of
config_directory_contents.
The code in read_config has been adapted from get_options, but there are
a number of behavior changes that I would rather not introduce:
- the order of config priority has changed
- user config from cmdline not allowed to be a dir anymore
- user config from cmdline does not /replace/ default user config, as it
used to
- no warning for nonsense profile names
- behavior of _update_loose does not match what the old code did. It
allows config to /override/ explicit command line options (e.g. --server)
- introduces a new error on unknown options (silently ignored before)
All that being said, I certainly think that library support for reading
config and managing profiles is needed. I'll see about fixing this
feature up.
> diff --git a/cli/koji b/cli/koji
> index 47d9691..66f4d99 100755
> --- a/cli/koji
> +++ b/cli/koji
> @@ -186,88 +186,9 @@ def get_options():
> list_commands()
> parser.error('Unknown command: %s' % args[0])
> assert False
> - # load local config
> - defaults = {
> - 'server' : 'http://localhost/kojihub',
> - 'weburl' : 'http://localhost/koji',
> - 'topurl' : None,
> - 'pkgurl' : None,
> - 'topdir' : '/mnt/koji',
> - 'max_retries' : None,
> - 'retry_interval': None,
> - 'anon_retry' : None,
> - 'offline_retry' : None,
> - 'offline_retry_interval' : None,
> - 'keepalive' : True,
> - 'timeout' : None,
> - 'use_fast_upload': False,
> - 'poll_interval': 5,
> - 'krbservice': 'host',
> - 'cert': '~/.koji/client.crt',
> - 'ca': '~/.koji/clientca.crt',
> - 'serverca': '~/.koji/serverca.crt',
> - 'authtype': None
> - }
> - #note: later config files override earlier ones
> - configs = koji.config_directory_contents('/etc/koji.conf.d')
> - if os.access('/etc/koji.conf', os.F_OK):
> - configs.append('/etc/koji.conf')
> - if options.configFile:
> - fn = os.path.expanduser(options.configFile)
> - if os.path.isdir(fn):
> - contents = koji.config_directory_contents(fn)
> - if not contents:
> - parser.error("No config files found in directory: %s" % fn)
> - configs.extend(contents)
> - else:
> - if not os.access(fn, os.F_OK):
> - parser.error("No such file: %s" % fn)
> - configs.append(fn)
> - else:
> - user_config_dir = os.path.expanduser("~/.koji/config.d")
> - configs.extend(koji.config_directory_contents(user_config_dir))
> - fn = os.path.expanduser("~/.koji/config")
> - if os.access(fn, os.F_OK):
> - configs.append(fn)
> - got_conf = False
> - for configFile in configs:
> - f = open(configFile)
> - config = ConfigParser.ConfigParser()
> - config.readfp(f)
> - f.close()
> - if config.has_section(options.profile):
> - got_conf = True
> - for name, value in config.items(options.profile):
> - #note the defaults dictionary also serves to indicate which
> - #options *can* be set via the config file. Such options should
> - #not have a default value set in the option parser.
> - if defaults.has_key(name):
> - if name in ('anon_retry', 'offline_retry', 'keepalive', 'use_fast_upload'):
> - defaults[name] = config.getboolean(options.profile, name)
> - elif name in ('max_retries', 'retry_interval',
> - 'offline_retry_interval', 'poll_interval', 'timeout'):
> - try:
> - defaults[name] = int(value)
> - except ValueError:
> - parser.error("value for %s config option must be a valid integer" % name)
> - assert False
> - else:
> - defaults[name] = value
> - if configs and not got_conf:
> - warn("Warning: no configuration for profile name: %s" % options.profile)
> - for name, value in defaults.iteritems():
> - if getattr(options, name, None) is None:
> - setattr(options, name, value)
> - dir_opts = ('topdir', 'cert', 'ca', 'serverca')
> - for name in dir_opts:
> - # expand paths here, so we don't have to worry about it later
> - value = os.path.expanduser(getattr(options, name))
> - setattr(options, name, value)
> -
> - #honor topdir
> - if options.topdir:
> - koji.BASEDIR = options.topdir
> - koji.pathinfo.topdir = options.topdir
> +
> + defaults = koji.read_config(options.profile, user_config=options.configFile)
> + options._update_loose(defaults.__dict__)
>
> #pkgurl is obsolete
> if options.pkgurl:
> diff --git a/koji/__init__.py b/koji/__init__.py
> index f45ff70..9951a28 100644
> --- a/koji/__init__.py
> +++ b/koji/__init__.py
> @@ -28,6 +28,7 @@ except ImportError:
> sys.stderr.write("Warning: Could not install krbV module. Kerberos support will be disabled.\n")
> sys.stderr.flush()
> import base64
> +import ConfigParser
> import datetime
> import errno
> from fnmatch import fnmatch
> @@ -35,6 +36,7 @@ import httplib
> import logging
> import logging.handlers
> from koji.util import md5_constructor
> +import optparse
> import os
> import os.path
> import pwd
> @@ -1457,6 +1459,110 @@ def config_directory_contents(dir_name):
> return configs
>
>
> +def _config_directory_contents(dir_name):
> + configs = []
> + try:
> + conf_dir_contents = os.listdir(dir_name)
> + except OSError, exception:
> + if exception.errno != errno.ENOENT:
> + raise
> + else:
> + for name in sorted(conf_dir_contents):
> + if not name.endswith('.conf'):
> + continue
> + config_full_name = os.path.join(dir_name, name)
> + configs.append(config_full_name)
> + return configs
> +
> +
> +def read_config(profile_name, user_config=None):
> + config_defaults = {
> + 'server': 'http://localhost/kojihub',
> + 'weburl': 'http://localhost/koji',
> + 'topurl': None,
> + 'pkgurl': None,
> + 'topdir': '/mnt/koji',
> + 'max_retries': None,
> + 'retry_interval': None,
> + 'anon_retry': None,
> + 'offline_retry': None,
> + 'offline_retry_interval': None,
> + 'keepalive': True,
> + 'timeout': None,
> + 'use_fast_upload': False,
> + 'poll_interval': 5,
> + 'krbservice': 'host',
> + 'cert': '~/.koji/client.crt',
> + 'ca': '~/.koji/clientca.crt',
> + 'serverca': '~/.koji/serverca.crt',
> + 'authtype': None
> + }
> +
> + int_options = ['max_retries', 'retry_interval', 'offline_retry_interval', 'poll_interval', 'timeout']
> + bool_options = ['anon_retry', 'offline_retry', 'keepalive', 'use_fast_upload']
> + path_options = ['topdir', 'cert', 'ca', 'serverca']
> +
> + result = config_defaults.copy()
> + for option in config_defaults:
> + if option in path_options:
> + result[option] = os.path.expanduser(result[option])
> +
> + configs = []
> +
> + # main config
> + configs.append("/etc/koji.conf")
> +
> + # conf.d
> + configs.extend(_config_directory_contents("/etc/koji.conf.d"))
> +
> + # user config
> + configs.append(os.path.expanduser("~/.koji/config"))
> +
> + # user conf.d
> + configs.extend(_config_directory_contents(os.path.expanduser("~/.koji/conf.d")))
> +
> + # TODO: read configs via xdg.BaseDirectory.load_config_path("koji")
> +
> + # user config specified in runtime
> + if user_config is not None:
> + configs.append(user_config)
> +
> + # read configs in particular order, use the last value found
> + for config_path in configs:
> + if not os.access(config_path, os.F_OK):
> + continue
> + config = ConfigParser.SafeConfigParser()
> + config.readfp(open(config_path, "r"))
> +
> + if profile_name not in config.sections():
> + continue
> +
> + # check for invalid options
> + invalid_options = []
> + for option in config.options(profile_name):
> + if option not in result:
> + invalid_options.append(option)
> +
> + if invalid_options:
> + raise ValueError("Invalid options: %s" % ", ".join(invalid_options))
> +
> + for option in config.options(profile_name):
> + if option in bool_options:
> + result[option] = config.getboolean(profile_name, option)
> + elif option in int_options:
> + result[option] = config.getint(profile_name, option)
> + else:
> + result[option] = config.get(profile_name, option)
> + if option in path_options:
> + result[option] = os.path.expanduser(result[option])
> +
> + result["profile"] = profile_name
> +
> + # convert dict to optparse Values
> + options = optparse.Values(result)
> + return options
> +
> +
> class PathInfo(object):
> # ASCII numbers and upper- and lower-case letter for use in tmpdir()
> ASCII_CHARS = [chr(i) for i in range(48, 58) + range(65, 91) + range(97, 123)]
>
More information about the buildsys
mailing list