PR#1296: extract read_config_files util for config parsing - koji

koji

#1296 extract read_config_files util for config parsing

Merged 4 years ago by tkopecek. Opened 5 years ago by julian8628.

julian8628/koji issue/865 into master

strict per item in config_files

Yuming Zhu • 4 years ago

7dc3b2d

fix test_runroot_builder tests

Yu Ming Zhu • 4 years ago

af44813

use RawConfigParser for kojid

Yu Ming Zhu • 4 years ago

98655e1

extract read_config_files util for config parsing

Yu Ming Zhu • 4 years ago

8120e03

builder/kojid

file modified

+1 -3

		`@@ -47,7 +47,6 @@`
		`import traceback`
		`import xml.dom.minidom`
		`import zipfile`
		`- from six.moves.configparser import RawConfigParser`
		`from fnmatch import fnmatch`
		`from gzip import GzipFile`
		`from optparse import OptionParser, SUPPRESS_HELP`
		`@@ -6100,8 +6099,7 @@`
		`assert False # pragma: no cover`

		`# load local config`
		`- config = RawConfigParser()`
		`- config.read(options.configFile)`
		`+ config = koji.read_config_files(options.configFile, raw=True)`
		`for x in config.sections():`
		`if x != 'kojid':`
		`quit('invalid section found in config file: %s' % x)`

cli/koji_cli/commands.py

file modified

+1 -3

		`@@ -5904,10 +5904,8 @@`
		`(task_options, args) = parser.parse_args(args)`

		`if task_options.config:`
		`- if not os.path.exists(task_options.config):`
		`- parser.error(_("%s not found!" % task_options.config))`
		`section = 'image-build'`
		`- config = koji.read_config_files(task_options.config)`
		`+ config = koji.read_config_files([(task_options.config, True)])`
		`if not config.has_section(section):`
		`parser.error(_("single section called [%s] is required" % section))`
		`# pluck out the positional arguments first`

hub/kojixmlrpc.py

file modified

+2 -12

		`@@ -20,7 +20,6 @@`

		`from __future__ import absolute_import`
		`from __future__ import division`
		`- from six.moves.configparser import RawConfigParser`
		`import datetime`
		`import inspect`
		`import logging`
		`@@ -396,17 +395,8 @@`
		`#get our config file(s)`
		`cf = environ.get('koji.hub.ConfigFile', '/etc/koji-hub/hub.conf')`
		`cfdir = environ.get('koji.hub.ConfigDir', '/etc/koji-hub/hub.conf.d')`
		`- if cfdir:`
		`- configs = koji.config_directory_contents(cfdir)`
		`- else:`
		`- configs = []`
		`- if cf and os.path.isfile(cf):`
		`- configs.append(cf)`
		`- if configs:`
		`- config = RawConfigParser()`
		`- config.read(configs)`
		`- else:`
		`- config = None`
		`+ config = koji.read_config_files([cfdir, (cf, True)], raw=True)`
		`+`
		`cfgmap = [`
		`#option, type, default`
		`['DBName', 'string', None],`

koji/__init__.py

file modified

+85 -55

		`@@ -1673,7 +1673,7 @@`
		`return fo`


		`- def config_directory_contents(dir_name):`
		`+ def config_directory_contents(dir_name, strict=False):`
		`configs = []`
		`try:`
		`conf_dir_contents = os.listdir(dir_name)`
		`@@ -1685,7 +1685,12 @@`
		`if not name.endswith('.conf'):`
		`continue`
		`config_full_name = os.path.join(dir_name, name)`
		`- configs.append(config_full_name)`
		`+ if os.path.isfile(config_full_name) \`
		`+ and os.access(config_full_name, os.F_OK):`
		`+ configs.append(config_full_name)`
		`+ elif strict:`
		`+ raise ConfigurationError("Config file %s can't be opened."`
		`+ % config_full_name)`
		`return configs`


		`@@ -1727,59 +1732,47 @@`
		`#note: later config files override earlier ones`

		`# /etc/koji.conf.d`
		`- configs = config_directory_contents('/etc/koji.conf.d')`
		`+ configs = ['/etc/koji.conf.d']`

		`# /etc/koji.conf`
		`- if os.access('/etc/koji.conf', os.F_OK):`
		`- configs.append('/etc/koji.conf')`
		`+ configs.append('/etc/koji.conf')`

		`# User specific configuration`
		`if user_config:`
		`# Config file specified on command line`
		`- fn = os.path.expanduser(user_config)`
		`- if os.path.isdir(fn):`
		`- # Specified config is a directory`
		`- contents = config_directory_contents(fn)`
		`- if not contents:`
		`- raise ConfigurationError("No config files found in directory: %s" % fn)`
		`- configs.extend(contents)`
		`- else:`
		`- # Specified config is a file`
		`- if not os.access(fn, os.F_OK):`
		`- raise ConfigurationError("No such file: %s" % fn)`
		`- configs.append(fn)`
		`+ # The existence will be checked`
		`+ configs.append((os.path.expanduser(user_config), True))`
		`else:`
		`- # User config`
		`- user_config_dir = os.path.expanduser("~/.koji/config.d")`
		`- configs.extend(config_directory_contents(user_config_dir))`
		`- fn = os.path.expanduser("~/.koji/config")`
		`- if os.access(fn, os.F_OK):`
		`- configs.append(fn)`
		`+ # User config dir`
		`+ configs.append(os.path.expanduser("~/.koji/config.d"))`
		`+ # User config file`
		`+ configs.append(os.path.expanduser("~/.koji/config"))`
		`+`
		`+ config = read_config_files(configs)`

		`# Load the configs in a particular order`
		`got_conf = False`
		`- for configFile in configs:`
		`- config = read_config_files(configFile)`
		`- if config.has_section(profile_name):`
		`- got_conf = True`
		`- for name, value in config.items(profile_name):`
		`- #note the config_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 name in result:`
		`- if name in ('anon_retry', 'offline_retry', 'use_fast_upload',`
		`- 'krb_rdns', 'debug', 'debug', 'debug_xmlrpc', 'krb_canon_host'):`
		`- result[name] = config.getboolean(profile_name, name)`
		`- elif name in ('max_retries', 'retry_interval',`
		`- 'offline_retry_interval', 'poll_interval',`
		`- 'timeout', 'auth_timeout',`
		`- 'upload_blocksize', 'pyver'):`
		`- try:`
		`- result[name] = int(value)`
		`- except ValueError:`
		`- raise ConfigurationError("value for %s config option must be a valid integer" % name)`
		`- else:`
		`- result[name] = value`
		`+ if config.has_section(profile_name):`
		`+ got_conf = True`
		`+ for name, value in config.items(profile_name):`
		`+ #note the config_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 name in result:`
		`+ if name in ('anon_retry', 'offline_retry',`
		`+ 'use_fast_upload', 'krb_rdns', 'debug',`
		`+ 'debug_xmlrpc', 'krb_canon_host'):`
		`+ result[name] = config.getboolean(profile_name, name)`
		`+ elif name in ('max_retries', 'retry_interval',`
		`+ 'offline_retry_interval', 'poll_interval',`
		`+ 'timeout', 'auth_timeout',`
		`+ 'upload_blocksize', 'pyver'):`
		`+ try:`
		`+ result[name] = int(value)`
		`+ except ValueError:`
		`+ raise ConfigurationError("value for %s config option must be a valid integer" % name)`
		`+ else:`
		`+ result[name] = value`

		`# Check if the specified profile had a config specified`
		`if configs and not got_conf:`
		`@@ -1855,14 +1848,41 @@`
		`def read_config_files(config_files, raw=False):`
		`"""Use parser to read config file(s)`

		`- :param config_files: config file(s) to read (required).`
		`- :type config_files: str or list`
		`+ :param config_files: config file(s) to read (required). Config file could`
		`+ be file or directory, and order is preserved.`
		`+ If it's a list/tuple of list/tuple, in each inner`
		`+ item, the 1st item is file/dir name, and the 2nd item`
		`+ is strict(False if not present), which indicate`
		`+ if checking that:`
		`+ 1. is dir an empty directory`
		`+ 2. dose file exist`
		`+ 3. is file accessible`
		`+ raising ConfigurationError if any above is True.`
		`+ :type config_files: str or list or tuple`
		`:param bool raw: enable 'raw' parsing (no interpolation). Default: False`

		`:return: object of parser which contains parsed content`
		`+`
		`+ :raises: GenericError: config_files is not valid`
		`+ ConfigurationError: See config_files if strict is true`
		`+ OSError: Directory in config_files is not accessible`
		`"""`
		`- if not isinstance(config_files, (list, tuple)):`
		`- config_files = [config_files]`
		`+ if isinstance(config_files, six.string_types):`
		`+ config_files = [(config_files, False)]`
		`+ elif isinstance(config_files, (list, tuple)):`
		`+ fcfgs = []`
		`+ for i in config_files:`
		`+ if isinstance(i, six.string_types):`
		`+ fcfgs.append((i, False))`
		`+ elif isinstance(i, (list, tuple)) and 0 < len(i) <= 2:`
		`+ fcfgs.append((i[0], i[1] if len(i) == 2 else False))`
		`+ else:`
		`+ raise GenericError('invalid value: %s or type: %s'`
		`+ % (i, type(i)))`
		`+ config_files = fcfgs`
		`+ else:`
		`+ raise GenericError('invalid type: %s' % type(config_files))`
		`+`
		`if raw:`
		`parser = six.moves.configparser.RawConfigParser`
		`elif six.PY2:`
		`@@ -1872,12 +1892,22 @@`
		`# deprecated alias`
		`parser = six.moves.configparser.ConfigParser`
		`config = parser()`
		`- for config_file in config_files:`
		`- with open(config_file, 'r') as f:`
		`- if six.PY2:`
		`- config.readfp(f)`
		`- else:`
		`- config.read_file(f)`
		`+ cfgs = []`
		`+ # append dir contents`
		`+ for config_file, strict in config_files:`
		`+ if os.path.isdir(config_file):`
		`+ fns = config_directory_contents(config_file, strict=strict)`
		`+ if fns:`
		`+ cfgs.extend(fns)`
		`+ elif strict:`
		`+ raise ConfigurationError("No config files found in directory:"`
		`+ " %s" % config_file)`
		`+ elif os.path.isfile(config_file) and os.access(config_file, os.F_OK):`
		`+ cfgs.append(config_file)`
		`+ elif strict:`
		`+ raise ConfigurationError("Config file %s can't be opened."`
		`+ % config_file)`
		`+ config.read(cfgs)`
		`return config`

plugins/builder/runroot.py

file modified

+1 -2

		`@@ -49,8 +49,7 @@`
		`return res`

		`def _read_config(self):`
		`- cp = six.moves.configparser.SafeConfigParser()`
		`- cp.read(CONFIG_FILE)`
		`+ cp = koji.read_config_files(CONFIG_FILE)`
		`self.config = {`
		`'default_mounts': [],`
		`'safe_roots': [],`

plugins/builder/save_failed_tree.py

file modified

+1 -3

		`@@ -3,7 +3,6 @@`
		`import os`
		`import sys`
		`import tarfile`
		`- import six.moves.configparser`

		`import koji`
		`import koji.tasks as tasks`
		`@@ -28,8 +27,7 @@`

		`def read_config():`
		`global config`
		`- cp = six.moves.configparser.SafeConfigParser()`
		`- cp.read(CONFIG_FILE)`
		`+ cp = koji.read_config_files(CONFIG_FILE)`
		`config = {`
		`'path_filters': [],`
		`'volume': None,`

plugins/hub/protonmsg.py

file modified

+1 -1

		`@@ -269,7 +269,7 @@`
		`log = logging.getLogger('koji.plugin.protonmsg')`
		`global CONFIG`
		`if not CONFIG:`
		`- CONFIG = koji.read_config_files(CONFIG_FILE)`
		`+ CONFIG = koji.read_config_files([(CONFIG_FILE, True)])`
		`urls = CONFIG.get('broker', 'urls').split()`
		`test_mode = False`
		`if CONFIG.has_option('broker', 'test_mode'):`

plugins/hub/rpm2maven.py

file modified

+4 -4

		`@@ -10,16 +10,15 @@`
		`from koji.context import context`
		`from koji.plugin import callback`
		`from koji.util import rmtree`
		`- import six.moves.configparser`
		`import fnmatch`
		`import os`
		`- import shutil`
		`import subprocess`

		`CONFIG_FILE = '/etc/koji-hub/plugins/rpm2maven.conf'`

		`config = None`

		`+`
		`@callback('postImport')`
		`def maven_import(cbtype, args, *kws):`
		`global config`
		`@@ -32,8 +31,7 @@`
		`filepath = kws['filepath']`

		`if not config:`
		`- config = six.moves.configparser.SafeConfigParser()`
		`- config.read(CONFIG_FILE)`
		`+ config = koji.read_config_files([(CONFIG_FILE, True)])`
		`name_patterns = config.get('patterns', 'rpm_names').split()`
		`for pattern in name_patterns:`
		`if fnmatch.fnmatch(rpminfo['name'], pattern):`
		`@@ -52,6 +50,7 @@`
		`if os.path.exists(tmpdir):`
		`rmtree(tmpdir)`

		`+`
		`def expand_rpm(filepath, tmpdir):`
		`devnull = open('/dev/null', 'r+')`
		`rpm2cpio = subprocess.Popen(['/usr/bin/rpm2cpio', filepath],`
		`@@ -69,6 +68,7 @@`
		`(filepath, rpm2cpio.wait(), cpio.wait()))`
		`devnull.close()`

		`+`
		`def scan_and_import(buildinfo, rpminfo, tmpdir):`
		`global config`
		`path_patterns = config.get('patterns', 'artifact_paths').split()`

plugins/hub/save_failed_tree.py

file modified

+1 -3

		`@@ -1,6 +1,5 @@`
		`from __future__ import absolute_import`
		`import sys`
		`- import six.moves.configparser`
		`import koji`
		`from koji.context import context`
		`from koji.plugin import export`
		`@@ -29,8 +28,7 @@`

		`# read configuration only once`
		`if config is None:`
		`- config = six.moves.configparser.SafeConfigParser()`
		`- config.read(CONFIG_FILE)`
		`+ config = koji.read_config_files([(CONFIG_FILE, True)])`
		`allowed_methods = config.get('permissions', 'allowed_methods').split()`
		`if len(allowed_methods) == 1 and allowed_methods[0] == '*':`
		`allowed_methods = '*'`

tests/test_cli/data/image-build-config-empty.conf

file added

		`@@ -0,0 +1,1 @@`
		`+ # empty config file`
		`\ No newline at end of file`

tests/test_cli/test_image_build.py

file modified

+18 -11

		`@@ -12,7 +12,10 @@`
		`from koji_cli.commands import handle_image_build, _build_image_oz`
		`from . import utils`

		`- ConfigParser = six.moves.configparser.ConfigParser`
		`+ if six.PY2:`
		`+ ConfigParser = six.moves.configparser.SafeConfigParser`
		`+ else:`
		`+ ConfigParser = six.moves.configparser.ConfigParser`

		`TASK_OPTIONS = {`
		`"background": None,`
		`@@ -240,28 +243,32 @@`
		`"""Test handle_image_build argument with --config cases"""`

		`# Case 1, config file not exist case`
		`- self.assert_system_exit(`
		`- handle_image_build,`
		`- self.options,`
		`- self.session,`
		`- ['--config', '/nonexistent-file-755684354'],`
		`- stderr=self.format_error_message('/nonexistent-file-755684354 not found!'),`
		`- activate_session=None)`
		`+ with self.assertRaises(koji.ConfigurationError) as cm:`
		`+ handle_image_build(self.options,`
		`+ self.session,`
		`+ ['--config', '/nonexistent-file-755684354'])`
		`+ self.assertEqual(cm.exception.args[0],`
		`+ "Config file /nonexistent-file-755684354 can't be opened.")`

		`- # Case 2, no image-build section in config file`

		`+ # Case 2, no image-build section in config file`
		`expected = "single section called [%s] is required" % "image-build"`

		`self.configparser.return_value = ConfigParser()`
		`+`
		`self.assert_system_exit(`
		`handle_image_build,`
		`self.options,`
		`self.session,`
		`- ['--config', '/dev/null'],`
		`+ ['--config',`
		`+ os.path.join(os.path.dirname(__file__),`
		`+ 'data/image-build-config-empty.conf')],`
		`stderr=self.format_error_message(expected),`
		`activate_session=None)`

		`- config_file = os.path.join(os.path.dirname(__file__), 'data/image-build-config.conf')`
		`+ config_file = os.path.join(os.path.dirname(__file__),`
		`+ 'data/image-build-config.conf')`
		`+ # Case 3, normal`
		`handle_image_build(`
		`self.options,`
		`self.session,`

tests/test_lib/test_utils.py

file modified

+150 -28

		`@@ -162,49 +162,171 @@`
		`class ConfigFileTestCase(unittest.TestCase):`
		`"""Test config file reading functions"""`

		`- @mock_open()`
		`- @mock.patch("six.moves.configparser.ConfigParser", spec=True)`
		`- @mock.patch("six.moves.configparser.RawConfigParser", spec=True)`
		`- @mock.patch("six.moves.configparser.SafeConfigParser", spec=True)`
		`- def test_read_config_files(self, scp_clz, rcp_clz, cp_clz, open_mock):`
		`+ def setUp(self):`
		`+ self.manager = mock.MagicMock()`
		`+ self.manager.logging = mock.patch('koji.logging').start()`
		`+ self.manager.isdir = mock.patch("os.path.isdir").start()`
		`+ self.manager.isfile = mock.patch("os.path.isfile").start()`
		`+ self.manager.access = mock.patch("os.access", return_value=True).start()`
		`+ self.manager.cp_clz = mock.patch("six.moves.configparser.ConfigParser",`
		`+ spec=True).start()`
		`+ self.manager.scp_clz = mock.patch("six.moves.configparser.SafeConfigParser",`
		`+ spec=True).start()`
		`+ self.manager.rcp_clz = mock.patch("six.moves.configparser.RawConfigParser",`
		`+ spec=True).start()`
		`+ if six.PY2:`
		`+ self.real_parser_clz = self.manager.scp_clz`
		`+ else:`
		`+ self.real_parser_clz = self.manager.cp_clz`
		`+ self.mocks = [self.manager.isdir,`
		`+ self.manager.isfile,`
		`+ self.manager.access,`
		`+ self.manager.open,`
		`+ self.manager.cp_clz,`
		`+ self.manager.scp_clz,`
		`+ self.manager.rcp_clz]`
		`+`
		`+ def reset_mock(self):`
		`+ for m in self.mocks:`
		`+ m.reset_mock()`
		`+`
		`+ def tearDown(self):`
		`+ mock.patch.stopall()`
		`+`
		`+ def test_read_config_files(self):`
		`+`
		`+ # bad config_files`
		`+ for files in [0,`
		`+ False,`
		`+ set(),`
		`+ dict(),`
		`+ object(),`
		`+ ('string', True),`
		`+ [('str', True, 'str')],`
		`+ [tuple()],]:`
		`+ with self.assertRaises(koji.GenericError):`
		`+ koji.read_config_files(files)`
		`+`
		`+ # string as config_files`
		`files = 'test1.conf'`
		`+ self.manager.isdir.return_value = False`
		`conf = koji.read_config_files(files)`
		`- open_mock.assert_called_once_with(files, 'r')`
		`+ self.manager.isdir.assert_called_once_with(files)`
		`if six.PY2:`
		`self.assertTrue(isinstance(conf,`
		`six.moves.configparser.SafeConfigParser.__class__))`
		`- scp_clz.assert_called_once()`
		`- scp_clz.return_value.readfp.assert_called_once()`
		`else:`
		`self.assertTrue(isinstance(conf,`
		`six.moves.configparser.ConfigParser.__class__))`
		`- cp_clz.assert_called_once()`
		`- cp_clz.return_value.read_file.assert_called_once()`
		`+ self.real_parser_clz.assert_called_once()`
		`+ self.real_parser_clz.return_value.read.assert_called_once_with([files])`

		`- open_mock.reset_mock()`
		`- cp_clz.reset_mock()`
		`- scp_clz.reset_mock()`
		`+ # list as config_files`
		`+ self.reset_mock()`
		`files = ['test1.conf', 'test2.conf']`
		`koji.read_config_files(files)`
		`- open_mock.assert_has_calls([call('test1.conf', 'r'),`
		`- call('test2.conf', 'r')],`
		`- any_order=True)`
		`- if six.PY2:`
		`- scp_clz.assert_called_once()`
		`- self.assertEqual(scp_clz.return_value.readfp.call_count, 2)`
		`- else:`
		`- cp_clz.assert_called_once()`
		`- self.assertEqual(cp_clz.return_value.read_file.call_count, 2)`

		`- open_mock.reset_mock()`
		`- cp_clz.reset_mock()`
		`- scp_clz.reset_mock()`
		`+ self.real_parser_clz.assert_called_once()`
		`+ self.real_parser_clz.return_value.read.assert_called_once_with(files)`
		`+`
		`+ # tuple as config_files`
		`+ self.reset_mock()`
		`+ files = ('test1.conf', 'test2.conf')`
		`+ koji.read_config_files(files)`
		`+`
		`+ # raw`
		`+ self.reset_mock()`
		`conf = koji.read_config_files(files, raw=True)`
		`self.assertTrue(isinstance(conf,`
		`six.moves.configparser.RawConfigParser.__class__))`
		`- cp_clz.assert_not_called()`
		`- scp_clz.assert_not_called()`
		`- rcp_clz.assert_called_once()`
		`+ self.manager.cp_clz.assert_not_called()`
		`+ self.manager.scp_clz.assert_not_called()`
		`+ self.manager.rcp_clz.assert_called_once()`
		`+`
		`+ # strict`
		`+ # case1, not a file`
		`+ self.reset_mock()`
		`+ files = [('test1.conf',), ('test2.conf', True)]`
		`+ self.manager.isfile.return_value = False`
		`+ with self.assertRaises(koji.ConfigurationError) as cm:`
		`+ koji.read_config_files(files)`
		`+ self.assertEqual(cm.exception.args[0],`
		`+ "Config file test2.conf can't be opened.")`
		`+`
		`+ self.assertEqual(self.manager.isdir.call_count, 2)`
		`+ self.assertEqual(self.manager.isfile.call_count, 2)`
		`+ self.manager.access.assert_not_called()`
		`+`
		`+ # case2, inaccessible`
		`+ self.reset_mock()`
		`+ self.manager.isfile.return_value = True`
		`+ self.manager.access.return_value = False`
		`+ with self.assertRaises(koji.ConfigurationError) as cm:`
		`+ koji.read_config_files(files)`
		`+ self.assertEqual(cm.exception.args[0],`
		`+ "Config file test2.conf can't be opened.")`
		`+ self.assertEqual(self.manager.isdir.call_count, 2)`
		`+ self.assertEqual(self.manager.isfile.call_count, 2)`
		`+ self.assertEqual(self.manager.access.call_count, 2)`
		`+`
		`+ # directories`
		`+ # strict==False`
		`+ self.reset_mock()`
		`+ files = ['test1.conf', 'gooddir', 'test2.conf', 'emptydir', 'nonexistdir']`
		`+ self.manager.isdir.side_effect = lambda f: False \`
		`+ if f in ['test1.conf', 'test2.conf', 'nonexistdir'] else True`
		`+ self.manager.isfile.side_effect = lambda f: False \`
		`+ if f in ['nonexistdir', 'gooddir/test1-4.dir.conf'] else True`
		`+ self.manager.access.return_value = True`
		`+ with mock.patch("os.listdir", side_effect=[['test1-2.conf',`
		`+ 'test1-1.conf',`
		`+ 'test1-3.txt',`
		`+ 'test1-4.dir.conf'],`
		`+ []]) as listdir_mock:`
		`+ conf = koji.read_config_files(files)`
		`+ listdir_mock.assert_has_calls([call('gooddir'), call('emptydir')])`
		`+ self.real_parser_clz.assert_called_once()`
		`+ self.real_parser_clz.return_value.read.assert_called_once_with(`
		`+ ['test1.conf',`
		`+ 'gooddir/test1-1.conf',`
		`+ 'gooddir/test1-2.conf',`
		`+ 'test2.conf'])`
		`+ self.assertEqual(self.manager.isdir.call_count, 5)`
		`+ self.assertEqual(self.manager.isfile.call_count, 6)`
		`+ self.assertEqual(self.manager.access.call_count, 4)`
		`+`
		`+ # strict==True`
		`+ # case1`
		`+ self.reset_mock()`
		`+ files[1] = ('gooddir', True)`
		`+ with mock.patch("os.listdir", return_value=['test1-2.conf',`
		`+ 'test1-1.conf',`
		`+ 'test1-3.txt',`
		`+ 'test1-4.dir.conf']`
		`+ ) as listdir_mock:`
		`+ with self.assertRaises(koji.ConfigurationError) as cm:`
		`+ conf = koji.read_config_files(files)`
		`+ self.assertEqual(cm.exception.args[0],`
		`+ "Config file gooddir/test1-4.dir.conf can't be"`
		`+ " opened.")`
		`+ listdir_mock.assert_called_once_with('gooddir')`
		`+`
		`+ # case2`
		`+ self.reset_mock()`
		`+ files[1] = ('gooddir', False)`
		`+ files[3] = ('emptydir', True)`
		`+ with mock.patch("os.listdir", side_effect=[['test1-2.conf',`
		`+ 'test1-1.conf',`
		`+ 'test1-3.txt',`
		`+ 'test1-4.dir.conf'],`
		`+ []]`
		`+ ) as listdir_mock:`
		`+ with self.assertRaises(koji.ConfigurationError) as cm:`
		`+ conf = koji.read_config_files(files)`
		`+ self.assertEqual(cm.exception.args[0],`
		`+ 'No config files found in directory: emptydir')`
		`+ self.assertEqual(listdir_mock.call_count, 2)`
		`+`


		`class MavenUtilTestCase(unittest.TestCase):`

tests/test_plugins/test_runroot_builder.py

file modified

+30 -24

		`@@ -24,6 +24,12 @@`
		`return mock.patch('builtins.open')`


		`+ if six.PY2:`
		`+ CONFIG_PARSER = 'six.moves.configparser.SafeConfigParser'`
		`+ else:`
		`+ CONFIG_PARSER = 'six.moves.configparser.ConfigParser'`
		`+`
		`+`
		`CONFIG1 = {`
		`'paths': {`
		`'default_mounts': '/mnt/archive,/mnt/workdir',`
		`@@ -96,11 +102,11 @@`


		`class TestRunrootConfig(unittest.TestCase):`
		`- @mock.patch('six.moves.configparser.SafeConfigParser')`
		`- def test_bad_config_paths0(self, safe_config_parser):`
		`+ @mock.patch(CONFIG_PARSER)`
		`+ def test_bad_config_paths0(self, config_parser):`
		`cp = FakeConfigParser()`
		`del cp.CONFIG['path0']['mountpoint']`
		`- safe_config_parser.return_value = cp`
		`+ config_parser.return_value = cp`
		`session = mock.MagicMock()`
		`options = mock.MagicMock()`
		`options.workdir = '/tmp/nonexistentdirectory'`
		`@@ -109,11 +115,11 @@`
		`self.assertEqual(cm.exception.args[0],`
		`"bad config: missing options in path0 section")`

		`- @mock.patch('six.moves.configparser.SafeConfigParser')`
		`- def test_bad_config_absolute_path(self, safe_config_parser):`
		`+ @mock.patch(CONFIG_PARSER)`
		`+ def test_bad_config_absolute_path(self, config_parser):`
		`cp = FakeConfigParser()`
		`cp.CONFIG['paths']['default_mounts'] = ''`
		`- safe_config_parser.return_value = cp`
		`+ config_parser.return_value = cp`
		`session = mock.MagicMock()`
		`options = mock.MagicMock()`
		`options.workdir = '/tmp/nonexistentdirectory'`
		`@@ -122,29 +128,29 @@`
		`self.assertEqual(cm.exception.args[0],`
		`"bad config: all paths (default_mounts, safe_roots, path_subs) needs to be absolute: ")`

		`- @mock.patch('six.moves.configparser.SafeConfigParser')`
		`- def test_valid_config(self, safe_config_parser):`
		`- safe_config_parser.return_value = FakeConfigParser()`
		`+ @mock.patch(CONFIG_PARSER)`
		`+ def test_valid_config(self, config_parser):`
		`+ config_parser.return_value = FakeConfigParser()`
		`session = mock.MagicMock()`
		`options = mock.MagicMock()`
		`options.workdir = '/tmp/nonexistentdirectory'`
		`runroot.RunRootTask(123, 'runroot', {}, session, options)`

		`- @mock.patch('six.moves.configparser.SafeConfigParser')`
		`- def test_valid_config_alt(self, safe_config_parser):`
		`- safe_config_parser.return_value = FakeConfigParser(CONFIG2)`
		`+ @mock.patch(CONFIG_PARSER)`
		`+ def test_valid_config_alt(self, config_parser):`
		`+ config_parser.return_value = FakeConfigParser(CONFIG2)`
		`session = mock.MagicMock()`
		`options = mock.MagicMock()`
		`options.workdir = '/tmp/nonexistentdirectory'`
		`runroot.RunRootTask(123, 'runroot', {}, session, options)`

		`- @mock.patch('six.moves.configparser.SafeConfigParser')`
		`- def test_pathnum_gaps(self, safe_config_parser):`
		`+ @mock.patch(CONFIG_PARSER)`
		`+ def test_pathnum_gaps(self, config_parser):`
		`session = mock.MagicMock()`
		`options = mock.MagicMock()`
		`options.workdir = '/tmp/nonexistentdirectory'`
		`config = CONFIG2.copy()`
		`- safe_config_parser.return_value = FakeConfigParser(config)`
		`+ config_parser.return_value = FakeConfigParser(config)`
		`task1 = runroot.RunRootTask(123, 'runroot', {}, session, options)`
		`# adjust the path numbers (but preserving order) and do it again`
		`config = CONFIG2.copy()`
		`@@ -152,29 +158,29 @@`
		`config['path999'] = config['path2']`
		`del config['path1']`
		`del config['path2']`
		`- safe_config_parser.return_value = FakeConfigParser(config)`
		`+ config_parser.return_value = FakeConfigParser(config)`
		`task2 = runroot.RunRootTask(123, 'runroot', {}, session, options)`
		`# resulting processed config should be the same`
		`self.assertEqual(task1.config, task2.config)`
		`paths = list([CONFIG2[k] for k in ('path0', 'path1', 'path2')])`
		`self.assertEqual(task2.config['paths'], paths)`

		`- @mock.patch('six.moves.configparser.SafeConfigParser')`
		`- def test_bad_path_sub(self, safe_config_parser):`
		`+ @mock.patch(CONFIG_PARSER)`
		`+ def test_bad_path_sub(self, config_parser):`
		`session = mock.MagicMock()`
		`options = mock.MagicMock()`
		`options.workdir = '/tmp/nonexistentdirectory'`
		`config = copy.deepcopy(CONFIG2)`
		`config['paths']['path_subs'] += 'incorrect:format'`
		`- safe_config_parser.return_value = FakeConfigParser(config)`
		`+ config_parser.return_value = FakeConfigParser(config)`
		`with self.assertRaises(koji.GenericError):`
		`runroot.RunRootTask(123, 'runroot', {}, session, options)`


		`class TestMounts(unittest.TestCase):`
		`- @mock.patch('six.moves.configparser.SafeConfigParser')`
		`- def setUp(self, safe_config_parser):`
		`- safe_config_parser.return_value = FakeConfigParser()`
		`+ @mock.patch(CONFIG_PARSER)`
		`+ def setUp(self, config_parser):`
		`+ config_parser.return_value = FakeConfigParser()`
		`self.session = mock.MagicMock()`
		`options = mock.MagicMock()`
		`options.workdir = '/tmp/nonexistentdirectory'`
		`@@ -324,8 +330,8 @@`
		`os_unlink.assert_not_called()`

		`class TestHandler(unittest.TestCase):`
		`- @mock.patch('six.moves.configparser.SafeConfigParser')`
		`- def setUp(self, safe_config_parser):`
		`+ @mock.patch(CONFIG_PARSER)`
		`+ def setUp(self, config_parser):`
		`self.session = mock.MagicMock()`
		`self.br = mock.MagicMock()`
		`self.br.mock.return_value = 0`

util/koji-gc

file modified

+46 -58

		`@@ -108,62 +108,50 @@`

		`defaults = parser.get_default_values()`

		`- config = six.moves.configparser.ConfigParser()`
		`- cf = getattr(options, 'config_file', None)`
		`- if cf:`
		`- if not os.access(cf, os.F_OK):`
		`- parser.error(_("No such file: %s") % cf)`
		`- assert False # pragma: no cover`
		`- else:`
		`- cf = '/etc/koji-gc/koji-gc.conf'`
		`- if not os.access(cf, os.F_OK):`
		`- cf = None`
		`- if not cf:`
		`- print("no config file")`
		`- config = None`
		`- else:`
		`- config.read(cf)`
		`- # List of values read from config file to update default parser values`
		`- cfgmap = [`
		`- # name, alias, type`
		`- ['keytab', None, 'string'],`
		`- ['principal', None, 'string'],`
		`- ['krbservice', None, 'string'],`
		`- ['krb_rdns', None, 'boolean'],`
		`- ['krb_canon_host', None, 'boolean'],`
		`- ['runas', None, 'string'],`
		`- ['user', None, 'string'],`
		`- ['password', None, 'string'],`
		`- ['noauth', None, 'boolean'],`
		`- ['cert', None, 'string'],`
		`- ['ca', None, 'string'], # FIXME: remove in next major release`
		`- ['serverca', None, 'string'],`
		`- ['server', None, 'string'],`
		`- ['weburl', None, 'string'],`
		`- ['smtp_host', None, 'string'],`
		`- ['from_addr', None, 'string'],`
		`- ['email_template', None, 'string'],`
		`- ['email_domain', None, 'string'],`
		`- ['mail', None, 'boolean'],`
		`- ['delay', None, 'string'],`
		`- ['unprotected_keys', None, 'string'],`
		`- ['grace_period', None, 'string'],`
		`- ['trashcan_tag', None, 'string'],`
		`- ['no_ssl_verify', None, 'boolean'],`
		`- ['timeout', None, 'integer'],`
		`- ]`
		`- for name, alias, type in cfgmap:`
		`- if alias is None:`
		`- alias = ('main', name)`
		`- if config.has_option(*alias):`
		`- if options.debug:`
		`- print("Using option %s from config file" % (alias,))`
		`- if type == 'integer':`
		`- setattr(defaults, name, config.getint(*alias))`
		`- elif type == 'boolean':`
		`- setattr(defaults, name, config.getboolean(*alias))`
		`- else:`
		`- setattr(defaults, name, config.get(*alias))`
		`+ cf = getattr(options, 'config_file', '/etc/koji-gc/koji-gc.conf')`
		`+ config = koji.read_config_files(cf)`
		`+`
		`+ # List of values read from config file to update default parser values`
		`+ cfgmap = [`
		`+ # name, alias, type`
		`+ ['keytab', None, 'string'],`
		`+ ['principal', None, 'string'],`
		`+ ['krbservice', None, 'string'],`
		`+ ['krb_rdns', None, 'boolean'],`
		`+ ['krb_canon_host', None, 'boolean'],`
		`+ ['runas', None, 'string'],`
		`+ ['user', None, 'string'],`
		`+ ['password', None, 'string'],`
		`+ ['noauth', None, 'boolean'],`
		`+ ['cert', None, 'string'],`
		`+ ['ca', None, 'string'], # FIXME: remove in next major release`
		`+ ['serverca', None, 'string'],`
		`+ ['server', None, 'string'],`
		`+ ['weburl', None, 'string'],`
		`+ ['smtp_host', None, 'string'],`
		`+ ['from_addr', None, 'string'],`
		`+ ['email_template', None, 'string'],`
		`+ ['email_domain', None, 'string'],`
		`+ ['mail', None, 'boolean'],`
		`+ ['delay', None, 'string'],`
		`+ ['unprotected_keys', None, 'string'],`
		`+ ['grace_period', None, 'string'],`
		`+ ['trashcan_tag', None, 'string'],`
		`+ ['no_ssl_verify', None, 'boolean'],`
		`+ ['timeout', None, 'integer'],`
		`+ ]`
		`+ for name, alias, type in cfgmap:`
		`+ if alias is None:`
		`+ alias = ('main', name)`
		`+ if config.has_option(*alias):`
		`+ if options.debug:`
		`+ print("Using option %s from config file" % (alias,))`
		`+ if type == 'integer':`
		`+ setattr(defaults, name, config.getint(*alias))`
		`+ elif type == 'boolean':`
		`+ setattr(defaults, name, config.getboolean(*alias))`
		`+ else:`
		`+ setattr(defaults, name, config.get(*alias))`
		`#parse again with defaults`
		`(options, args) = parser.parse_args(values=defaults)`
		`options.config = config`
		`@@ -187,8 +175,8 @@`
		`#parse key aliases`
		`options.key_aliases = {}`
		`try:`
		`- if config and config.has_option('main', 'key_aliases'):`
		`- for line in config.get('main','key_aliases').splitlines():`
		`+ if config.has_option('main', 'key_aliases'):`
		`+ for line in config.get('main', 'key_aliases').splitlines():`
		`parts = line.split()`
		`if len(parts) < 2:`
		`continue`

util/koji-shadow

file modified

+38 -51

		`@@ -31,7 +31,6 @@`
		`krbV = None`
		`import koji`
		`from koji.util import to_list`
		`- import six.moves.configparser`
		`import fnmatch`
		`import optparse`
		`import os`
		`@@ -154,56 +153,44 @@`
		`(options, args) = parser.parse_args()`

		`defaults = parser.get_default_values()`
		`- config = six.moves.configparser.ConfigParser()`
		`- cf = getattr(options, 'config_file', None)`
		`- if cf:`
		`- if not os.access(cf, os.F_OK):`
		`- parser.error(_("No such file: %s") % cf)`
		`- assert False # pragma: no cover`
		`- else:`
		`- cf = '/etc/koji-shadow/koji-shadow.conf'`
		`- if not os.access(cf, os.F_OK):`
		`- cf = None`
		`- if not cf:`
		`- log("no config file")`
		`- config = None`
		`- else:`
		`- config.read(cf)`
		`- #allow config file to update defaults`
		`- for opt in parser.option_list:`
		`- if not opt.dest:`
		`- continue`
		`- name = opt.dest`
		`- alias = ('main', name)`
		`- if config.has_option(*alias):`
		`- log("Using option %s from config file" % (alias,))`
		`- if opt.action in ('store_true', 'store_false'):`
		`- setattr(defaults, name, config.getboolean(*alias))`
		`- elif opt.action != 'store':`
		`- pass`
		`- elif opt.type in ('int', 'long'):`
		`- setattr(defaults, name, config.getint(*alias))`
		`- elif opt.type in ('float'):`
		`- setattr(defaults, name, config.getfloat(*alias))`
		`- else:`
		`- log(config.get(*alias))`
		`- setattr(defaults, name, config.get(*alias))`
		`- #config file options without a cmdline equivalent`
		`- otheropts = [`
		`- #name, type, default`
		`- ['keytab', None, 'string'],`
		`- ['principal', None, 'string'],`
		`- ['runas', None, 'string'],`
		`- ['user', None, 'string'],`
		`- ['password', None, 'string'],`
		`- ['noauth', None, 'boolean'],`
		`- ['server', None, 'string'],`
		`- ['remote', None, 'string'],`
		`- ['max_jobs', None, 'int'],`
		`- ['serverca', None, 'string'],`
		`- ['auth_cert', None, 'string'],`
		`- ['arches', None, 'string'],`
		`- ]`
		`+ cf = getattr(options, 'config_file', '/etc/koji-shadow/koji-shadow.conf')`
		`+ config = koji.read_config_files(cf)`
		`+`
		`+ #allow config file to update defaults`
		`+ for opt in parser.option_list:`
		`+ if not opt.dest:`
		`+ continue`
		`+ name = opt.dest`
		`+ alias = ('main', name)`
		`+ if config.has_option(*alias):`
		`+ log("Using option %s from config file" % (alias,))`
		`+ if opt.action in ('store_true', 'store_false'):`
		`+ setattr(defaults, name, config.getboolean(*alias))`
		`+ elif opt.action != 'store':`
		`+ pass`
		`+ elif opt.type in ('int', 'long'):`
		`+ setattr(defaults, name, config.getint(*alias))`
		`+ elif opt.type in ('float'):`
		`+ setattr(defaults, name, config.getfloat(*alias))`
		`+ else:`
		`+ log(config.get(*alias))`
		`+ setattr(defaults, name, config.get(*alias))`
		`+ #config file options without a cmdline equivalent`
		`+ otheropts = [`
		`+ #name, type, default`
		`+ ['keytab', None, 'string'],`
		`+ ['principal', None, 'string'],`
		`+ ['runas', None, 'string'],`
		`+ ['user', None, 'string'],`
		`+ ['password', None, 'string'],`
		`+ ['noauth', None, 'boolean'],`
		`+ ['server', None, 'string'],`
		`+ ['remote', None, 'string'],`
		`+ ['max_jobs', None, 'int'],`
		`+ ['serverca', None, 'string'],`
		`+ ['auth_cert', None, 'string'],`
		`+ ['arches', None, 'string'],`
		`+ ]`


		`#parse again with updated defaults`

util/kojira

file modified

+1 -3

		`@@ -27,7 +27,6 @@`
		`import koji`
		`from koji.util import rmtree, parseStatus, to_list`
		`from optparse import OptionParser`
		`- from six.moves.configparser import ConfigParser`
		`import errno`
		`import json`
		`import logging`
		`@@ -901,8 +900,7 @@`
		`parser.add_option("--logfile", help="Specify logfile")`
		`(options, args) = parser.parse_args()`

		`- config = ConfigParser()`
		`- config.read(options.configFile)`
		`+ config = koji.read_config_files(options.configFile)`
		`section = 'kojira'`
		`for x in config.sections():`
		`if x != section:`

vm/kojikamid.py

file modified

+5 -2

		`@@ -28,7 +28,7 @@`

		`from __future__ import absolute_import`
		`from optparse import OptionParser`
		`- from six.moves.configparser import ConfigParser`
		`+ from six.moves.configparser import ConfigParser, SafeConfigParser`
		`import os`
		`import subprocess`
		`import sys`
		`@@ -212,7 +212,10 @@`
		`elif len(specfiles) > 1:`
		`raise BuildError('Multiple .ini files found')`

		`- conf = ConfigParser()`
		`+ if six.PY2:`
		`+ conf = SafeConfigParser()`
		`+ else:`
		`+ conf = ConfigParser()`
		`conf.read(os.path.join(self.spec_dir, specfiles[0]))`

		`# [naming] section`

vm/kojivmd

file modified

+1 -3

		`@@ -46,7 +46,6 @@`
		`import pwd`
		`import requests`
		`import fnmatch`
		`- from six.moves.configparser import ConfigParser`
		`from contextlib import closing`
		`from optparse import OptionParser`
		`try:`
		`@@ -102,8 +101,7 @@`
		`assert False # pragma: no cover`

		`# load local config`
		`- config = ConfigParser()`
		`- config.read(options.configFile)`
		`+ config = koji.read_config_files(options.configFile)`
		`for x in config.sections():`
		`if x != 'kojivmd':`
		`quit('invalid section found in config file: %s' % x)`

www/kojiweb/wsgi_publisher.py

file modified

+2 -13

		`@@ -30,7 +30,6 @@`
		`import sys`
		`import traceback`

		`- from six.moves.configparser import RawConfigParser`
		`from koji.server import ServerError, ServerRedirect`
		`from koji.util import dslice, to_list`
		`import six`
		`@@ -127,22 +126,12 @@`
		`"""`
		`cf = environ.get('koji.web.ConfigFile', '/etc/kojiweb/web.conf')`
		`cfdir = environ.get('koji.web.ConfigDir', '/etc/kojiweb/web.conf.d')`
		`- if cfdir:`
		`- configs = koji.config_directory_contents(cfdir)`
		`- else:`
		`- configs = []`
		`- if cf and os.path.isfile(cf):`
		`- configs.append(cf)`
		`- if configs:`
		`- config = RawConfigParser()`
		`- config.read(configs)`
		`- else:`
		`- raise koji.GenericError("Configuration missing")`
		`+ config = koji.read_config_files([cfdir, (cf, True)])`

		`opts = {}`
		`for name, dtype, default in self.cfgmap:`
		`key = ('web', name)`
		`- if config and config.has_option(*key):`
		`+ if config.has_option(*key):`
		`if dtype == 'integer':`
		`opts[name] = config.getint(*key)`
		`elif dtype == 'boolean':`

julian8628 commented 5 years ago

fixes: #865
based on PR#866 and PR#1150

extend read_config_files with strict option and directory support
use ConfigParser for all invokings except kojixmlrpc

ngompa commented 5 years ago

:thumbsup:

rebased onto 7ea77b549e969fdb6f2178c9319cac6f543c0c94

4 years ago

1 new commit added

fix test_runroot_builder tests

4 years ago

julian8628 commented 4 years ago

change ConfigParser to RawConfigParser for kojid, related to PR #1543

cc @tkopecek

tkopecek commented 4 years ago

@julian8628 Can you rebase this one?

rebased onto 8d5525e47b9dc7108d2c7aa6c77473694a09c90a

4 years ago

julian8628 commented 4 years ago

rebased

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

4 years ago

rebased onto 8120e03

4 years ago

julian8628 commented 4 years ago

add some tweaks
- remove default argument. The first config in config_files is actually the default one
- drop strict option for all config files and dirs
- change config_files argument, it could be a list of string and tuple, the second item in the tuple is actualyl strict option for this config file/dir(the 1st item).

In previous code, "global" strict option is too strict.

tkopecek commented 4 years ago

:thumbsup: