PR#1681: Improve handling of configuration file - fm-orchestrator

fm-orchestrator

#1681 Improve handling of configuration file

Merged a year ago by breilly. Opened 3 years ago by otaylor.

otaylor/fm-orchestrator config-file-errors into master

Improve handling of configuration file

Owen W. Taylor • a year ago

b6c11f8

README.rst

file modified

+3 -1

		`@@ -903,7 +903,9 @@`
		`recognized:`

		- ``MBS_CONFIG_FILE``: Overrides default configuration file location,
		- typically ``/etc/module-build-service/config.py``.
		+ typically ``/etc/module-build-service/config.py``. If set to the
		`+ empty string, no configuration file will be read and default`
		`+ values will be used.`
		- ``MBS_CONFIG_SECTION``: Overrides configuration section.

		It is possible to set these values in httpd using ``SetEnv``,

module_build_service/common/config.py

file modified

+19 -6

		`@@ -1,6 +1,7 @@`
		`# -- coding: utf-8 --`
		`# SPDX-License-Identifier: MIT`
		`from __future__ import absolute_import`
		`+ import errno`
		`import imp`
		`import logging`
		`import os`
		`@@ -108,15 +109,27 @@`
		`to configure Flask with.`
		`:rtype: tuple(Config, object)`
		`"""`
		`- config_file = os.environ.get("MBS_CONFIG_FILE", "/etc/module-build-service/config.py")`
		`+ env_config_file = os.environ.get("MBS_CONFIG_FILE")`
		`+ if env_config_file is None:`
		`+ config_file = "/etc/module-build-service/config.py"`
		`+ else:`
		`+ config_file = env_config_file`
		`+`
		`+ config_module = None`
		`+`
		`+ # MBS_CONFIG_FILE="" entirely suppresses looking a config file`
		`+ if config_file != "":`
		`+ try:`
		`+ config_module = imp.load_source("mbs_runtime_config", config_file)`
		`+ log.info("Using the configuration file at %s", config_file)`
		`+ except OSError as e:`
		`+ if e.errno != errno.ENOENT or env_config_file:`
		`+ log.error("Can't open config file: %s", e)`

		`- try:`
		`- config_module = imp.load_source("mbs_runtime_config", config_file)`
		`- log.info("Using the configuration file at %s", config_file)`
		`- except Exception:`
		`- log.warning("The configuration file at %s was not present", config_file)`
		`+ if config_module is None:`
		`# Default to this file as the configuration module`
		`config_module = sys.modules[__name__]`
		`+ log.debug("Using default configuration")`

		`true_options = ("1", "on", "true", "y", "yes")`
		`if any(["py.test" in arg or "pytest" in arg for arg in sys.argv]):`

otaylor commented 3 years ago

Right now, if there is no /etc/module-build-service/config.py (which is the expected situation for local builds against Fedora), a warning is printed at startup. The same warning is printed if loading the config file throws any sort of exception.

Allow MBS_CONFIG_FILE="" to entirely suppress loading any configuration file (useful for running tests and avoiding loading a system-wide configuration file.)
When loading the configuration file:
- If the default configuration file path doesn't exist, silently fall back to the default configuration
- For any other OSError, print the exact error
- Let any other exception throw through, to allow people to debug their
  configuration file

Note: not on my critical path - has just been bugging me for a while (and confusing people trying to use the new module-build-service on Fedora, though that's further confused because there is a stray /etc/module-build-service/config.py packaged in the RPM.)

mkovarik commented 3 years ago

Build 82b13af116a4f1aaf9e37cd57c5e495d2fcd9b06 FAILED!
Rebase or make new commits to rebuild.

otaylor commented 3 years ago

Should note that the no-config-file-by-default setup makes it hard to figure out how to change config variables - one of the developers I work with needed to reduce the number of simultaneous local builds, which involved creating a /etc/module-build-service/config.py with:

from module_build_service.common.config import
LocalBuildConfiguration as DefaultLocalBuildConfiguration

class LocalBuildConfiguration(DefaultLocalBuildConfiguration):
      NUM_THREADS_FOR_BUILD_SUBMISSIONS = 1

So there's some argument that an empty file with that structure should be created by default. But given the current setup, this PR would be an improvement.

Edited 3 years ago by otaylor