[aur-dev][PATCH] config: allow reading both the proto file and the modified config
This change allows aurweb configuration to be done via either: - copying config.proto to config and modifying values - creating a new config only containing modified values, next to a config.proto containing unmodified values The motivation for this change is to enable ansible configuration by storing only changed values, and deferring to config.proto otherwise. If a config.proto file does not exist next to /etc/aurweb/config or $AUR_CONFIG, it is ignored and *all* values are expected to live in the modified config file. Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> --- aurweb/config.py | 4 ++++ web/lib/confparser.inc.php | 8 +++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/aurweb/config.py b/aurweb/config.py index a52d942..c333c43 100644 --- a/aurweb/config.py +++ b/aurweb/config.py @@ -13,6 +13,10 @@ def _get_parser(): path = os.environ.get('AUR_CONFIG') else: path = "/etc/aurweb/config" + + if os.path.isfile(path + '.proto'): + with open(path + '.proto') as f: + _parser.read_file(f) _parser.read(path) return _parser diff --git a/web/lib/confparser.inc.php b/web/lib/confparser.inc.php index 499481d..2ed0108 100644 --- a/web/lib/confparser.inc.php +++ b/web/lib/confparser.inc.php @@ -8,11 +8,17 @@ function config_load() { if (!$path) { $path = "/etc/aurweb/config"; } + if (file_exists($path . ".proto")) { + $defaults = parse_ini_file($path . ".proto", true, INI_SCANNER_RAW); + } else { + $defaults = []; + } if (file_exists($path)) { - $AUR_CONFIG = parse_ini_file($path, true, INI_SCANNER_RAW); + $config = parse_ini_file($path, true, INI_SCANNER_RAW); } else { die("aurweb config file not found"); } + $AUR_CONFIG = array_replace_recursive($defaults, $config) } } -- 2.17.0
What do you think about adding one more line to check if $config is already set? Something like: if (!isset($config)) { ... do the parse_ini_file } Just to reduce disk operations. On Thu, Apr 12, 2018 at 10:44 AM, Eli Schwartz <eschwartz@archlinux.org> wrote:
This change allows aurweb configuration to be done via either: - copying config.proto to config and modifying values - creating a new config only containing modified values, next to a config.proto containing unmodified values
The motivation for this change is to enable ansible configuration by storing only changed values, and deferring to config.proto otherwise.
If a config.proto file does not exist next to /etc/aurweb/config or $AUR_CONFIG, it is ignored and *all* values are expected to live in the modified config file.
Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> --- aurweb/config.py | 4 ++++ web/lib/confparser.inc.php | 8 +++++++- 2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/aurweb/config.py b/aurweb/config.py index a52d942..c333c43 100644 --- a/aurweb/config.py +++ b/aurweb/config.py @@ -13,6 +13,10 @@ def _get_parser(): path = os.environ.get('AUR_CONFIG') else: path = "/etc/aurweb/config" + + if os.path.isfile(path + '.proto'): + with open(path + '.proto') as f: + _parser.read_file(f) _parser.read(path)
return _parser diff --git a/web/lib/confparser.inc.php b/web/lib/confparser.inc.php index 499481d..2ed0108 100644 --- a/web/lib/confparser.inc.php +++ b/web/lib/confparser.inc.php @@ -8,11 +8,17 @@ function config_load() { if (!$path) { $path = "/etc/aurweb/config"; } + if (file_exists($path . ".proto")) { + $defaults = parse_ini_file($path . ".proto", true, INI_SCANNER_RAW); + } else { + $defaults = []; + } if (file_exists($path)) { - $AUR_CONFIG = parse_ini_file($path, true, INI_SCANNER_RAW); + $config = parse_ini_file($path, true, INI_SCANNER_RAW); } else { die("aurweb config file not found"); } + $AUR_CONFIG = array_replace_recursive($defaults, $config) } }
-- 2.17.0
On 04/12/2018 01:51 PM, Nodiv Byzero wrote:
What do you think about adding one more line to check if $config is already set? Something like: if (!isset($config)) { ... do the parse_ini_file }
Just to reduce disk operations.
What is the point? We only check if $AUR_CONFIG isset() because it is explicitly global, and the function is here to set it if needed or else do nothing. $config is locally scoped, and its only purpose is exactly where it is being used. It should be utterly impossible for it to be set beforehand. ... On an unrelated note, Bluewind suggested renaming the config.proto to config.defaults, so if Lukas agrees with this patch I will send in an amended version. -- Eli Schwartz Bug Wrangler and Trusted User
The point is that there are few places where config_load() gets called in the sequential order (the same request) and every time it loads and parses data. On Thu, Apr 12, 2018 at 11:05 AM, Eli Schwartz <eschwartz@archlinux.org> wrote:
On 04/12/2018 01:51 PM, Nodiv Byzero wrote:
What do you think about adding one more line to check if $config is already set? Something like: if (!isset($config)) { ... do the parse_ini_file }
Just to reduce disk operations.
What is the point? We only check if $AUR_CONFIG isset() because it is explicitly global, and the function is here to set it if needed or else do nothing.
$config is locally scoped, and its only purpose is exactly where it is being used. It should be utterly impossible for it to be set beforehand.
...
On an unrelated note, Bluewind suggested renaming the config.proto to config.defaults, so if Lukas agrees with this patch I will send in an amended version.
-- Eli Schwartz Bug Wrangler and Trusted User
On 04/12/2018 02:22 PM, Nodiv Byzero wrote:
The point is that there are few places where config_load() gets called in the sequential order (the same request) and every time it loads and parses data.
Are these also places where if (!isset($AUR_CONFIG)) fails to actually fulfill its sole reason to exist? Not that it matters... if for some reason we are loading the config very fast and scribbling over each other, because $AUR_CONFIG is not yet set when we begin the second config_load() execution, checking if $config is already loaded won't help -- it's a local variable. So, question: can you show me code where $config would *not* be `false`? Because I think this proposition would be either a no-op or a reason to file a severely major upstream php bug complaining that the variables are leaking all over the floor. ... That being said, yes, there is a race condition where you call config_load() twice, and they both set $AUR_CONFIG. Elevating $config and $default_config to globals, *then* using them as additional file access caches to micro-optimize one or two disk ops, seems wasteful to me. It's not like this is being run in a tight loop. If any effort should be spent to fix this, rather than introducing painful, non-obvious code, we should introduce some sort of proper cache. -- Eli Schwartz Bug Wrangler and Trusted User
I think you're right. I looked one more time and found guard there: if (!isset($AUR_CONFIG)) Thanks for clarification. Nodiv. On Mon, Apr 16, 2018 at 1:41 AM, Eli Schwartz <eschwartz@archlinux.org> wrote:
On 04/12/2018 02:22 PM, Nodiv Byzero wrote:
The point is that there are few places where config_load() gets called in the sequential order (the same request) and every time it loads and parses data.
Are these also places where if (!isset($AUR_CONFIG)) fails to actually fulfill its sole reason to exist?
Not that it matters... if for some reason we are loading the config very fast and scribbling over each other, because $AUR_CONFIG is not yet set when we begin the second config_load() execution, checking if $config is already loaded won't help -- it's a local variable.
So, question: can you show me code where $config would *not* be `false`?
Because I think this proposition would be either a no-op or a reason to file a severely major upstream php bug complaining that the variables are leaking all over the floor.
...
That being said, yes, there is a race condition where you call config_load() twice, and they both set $AUR_CONFIG.
Elevating $config and $default_config to globals, *then* using them as additional file access caches to micro-optimize one or two disk ops, seems wasteful to me. It's not like this is being run in a tight loop.
If any effort should be spent to fix this, rather than introducing painful, non-obvious code, we should introduce some sort of proper cache.
-- Eli Schwartz Bug Wrangler and Trusted User
On Thu, 12 Apr 2018 at 19:44:15, Eli Schwartz wrote:
This change allows aurweb configuration to be done via either: - copying config.proto to config and modifying values - creating a new config only containing modified values, next to a config.proto containing unmodified values
The motivation for this change is to enable ansible configuration by storing only changed values, and deferring to config.proto otherwise.
If a config.proto file does not exist next to /etc/aurweb/config or $AUR_CONFIG, it is ignored and *all* values are expected to live in the modified config file.
Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> --- aurweb/config.py | 4 ++++ web/lib/confparser.inc.php | 8 +++++++- 2 files changed, 11 insertions(+), 1 deletion(-) [...]
Thanks, I like the idea. However, I think it would be better to make both the path to the defaults file and the path to the configuration file itself configurable (i.e. add a new environment variable AUR_CONFIG_DEFAULTS). By default, /etc/aurweb/config.defaults and /etc/aurweb/config could be used. Best regards, Lukas
On Sat, 14 Apr 2018 at 16:04:58, Lukas Fleischer wrote:
On Thu, 12 Apr 2018 at 19:44:15, Eli Schwartz wrote:
This change allows aurweb configuration to be done via either: - copying config.proto to config and modifying values - creating a new config only containing modified values, next to a config.proto containing unmodified values
The motivation for this change is to enable ansible configuration by storing only changed values, and deferring to config.proto otherwise.
If a config.proto file does not exist next to /etc/aurweb/config or $AUR_CONFIG, it is ignored and *all* values are expected to live in the modified config file.
Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> --- aurweb/config.py | 4 ++++ web/lib/confparser.inc.php | 8 +++++++- 2 files changed, 11 insertions(+), 1 deletion(-) [...]
Thanks, I like the idea. However, I think it would be better to make both the path to the defaults file and the path to the configuration file itself configurable (i.e. add a new environment variable AUR_CONFIG_DEFAULTS). By default, /etc/aurweb/config.defaults and /etc/aurweb/config could be used.
Maybe even better: make AUR_CONFIG default to /etc/aurweb/config and AUR_CONFIG_DEFAULTS default to the value of AUR_CONFIG with an additional ".defaults" suffix. Then, if no environment variables are set, aurweb would look for default values in /etc/aurweb/config.defaults and for additional configuration in /etc/aurweb/config. If only AUR_CONFIG is set (say, to /some/path), aurweb looks for default values under /some/path.defaults and for additional configuration under /some/path. And if you really want to store the defaults somewhere else, you can set AUR_CONFIG_DEFAULTS. Best regards, Lukas Fleischer
On 04/15/2018 06:11 AM, Lukas Fleischer wrote:
Maybe even better: make AUR_CONFIG default to /etc/aurweb/config and AUR_CONFIG_DEFAULTS default to the value of AUR_CONFIG with an additional ".defaults" suffix.
Great minds think alike. :) -- Eli Schwartz Bug Wrangler and Trusted User
In the process, rename config.proto to config.defaults (because that is what it is now). Also use dict.get('key', default_value) when querying os.environ, rather than an if block, as it is more pythonic/readable/concise, and reduces the number of dict lookups. This change allows aurweb configuration to be done via either: - copying config.defaults to config and modifying values - creating a new config only containing modified values, next to a config.defaults containing unmodified values The motivation for this change is to enable ansible configuration in our flagship deployment by storing only changed values, and deferring to config.defaults otherwise. A side benefit is, it is easier to see what has changed by inspecting only the site configuration file. If a config.defaults file does not exist next to $AUR_CONFIG or in $AUR_CONFIG_DEFAULTS, it is ignored and *all* values are expected to live in the modified config file. Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> --- v2: I opted to make the default location of the defaults config dependent on the location of the site config, since I think that makes more sense... Also reimplement the env getting code on python to be more pythonic INSTALL | 6 ++++-- TESTING | 2 +- aurweb/config.py | 10 ++++++---- conf/{config.proto => config.defaults} | 0 web/lib/confparser.inc.php | 12 +++++++++++- 5 files changed, 22 insertions(+), 8 deletions(-) rename conf/{config.proto => config.defaults} (100%) diff --git a/INSTALL b/INSTALL index c72c4a2..7170aea 100644 --- a/INSTALL +++ b/INSTALL @@ -40,8 +40,10 @@ read the instructions below. Ensure to enable the pdo_mysql extension in php.ini. -3) Copy conf/config.proto to /etc/aurweb/config and adjust the configuration - (pay attention to disable_http_login, enable_maintenance and aur_location). +3) Optionally copy conf/config.defaults to /etc/aurweb/. Create or copy + /etc/aurweb/config (this is expected to contain all configuration settings + if the defaults file does not exist) and adjust the configuration (pay + attention to disable_http_login, enable_maintenance and aur_location). 4) Create a new MySQL database and a user and import the aurweb SQL schema: diff --git a/TESTING b/TESTING index 53ffef2..b0a5f62 100644 --- a/TESTING +++ b/TESTING @@ -23,7 +23,7 @@ INSTALL. $ sqlite3 ../aurweb.sqlite3 < aur-schema-sqlite.sql $ sqlite3 ../aurweb.sqlite3 < out.sql -4) Copy conf/config.proto to conf/config and adjust the configuration +4) Copy conf/config.defaults to conf/config and adjust the configuration (pay attention to disable_http_login, enable_maintenance and aur_location). Be sure to change backend to sqlite and name to the file location of your diff --git a/aurweb/config.py b/aurweb/config.py index a52d942..52ec461 100644 --- a/aurweb/config.py +++ b/aurweb/config.py @@ -8,11 +8,13 @@ def _get_parser(): global _parser if not _parser: + path = os.environ.get('AUR_CONFIG', '/etc/aurweb/config') + defaults = os.environ.get('AUR_CONFIG_DEFAULTS', path + '.defaults') + _parser = configparser.RawConfigParser() - if 'AUR_CONFIG' in os.environ: - path = os.environ.get('AUR_CONFIG') - else: - path = "/etc/aurweb/config" + if os.path.isfile(defaults): + with open(defaults) as f: + _parser.read_file(f) _parser.read(path) return _parser diff --git a/conf/config.proto b/conf/config.defaults similarity index 100% rename from conf/config.proto rename to conf/config.defaults diff --git a/web/lib/confparser.inc.php b/web/lib/confparser.inc.php index 499481d..29f17e8 100644 --- a/web/lib/confparser.inc.php +++ b/web/lib/confparser.inc.php @@ -8,11 +8,21 @@ function config_load() { if (!$path) { $path = "/etc/aurweb/config"; } + $defaults_path = getenv('AUR_CONFIG_DEFAULTS'); + if (!$defaults_path) { + $defaults_path = path . ".defaults"; + } + if (file_exists($defaults_path)) { + $default_config = parse_ini_file($defaults_path, true, INI_SCANNER_RAW); + } else { + $default_config = []; + } if (file_exists($path)) { - $AUR_CONFIG = parse_ini_file($path, true, INI_SCANNER_RAW); + $config = parse_ini_file($path, true, INI_SCANNER_RAW); } else { die("aurweb config file not found"); } + $AUR_CONFIG = array_replace_recursive($default_config, $config) } } -- 2.17.0
On Sun, 15 Apr 2018 at 16:29:43, Eli Schwartz wrote:
In the process, rename config.proto to config.defaults (because that is what it is now).
Also use dict.get('key', default_value) when querying os.environ, rather than an if block, as it is more pythonic/readable/concise, and reduces the number of dict lookups.
This change allows aurweb configuration to be done via either: - copying config.defaults to config and modifying values - creating a new config only containing modified values, next to a config.defaults containing unmodified values [...] INSTALL | 6 ++++-- TESTING | 2 +- aurweb/config.py | 10 ++++++---- conf/{config.proto => config.defaults} | 0 web/lib/confparser.inc.php | 12 +++++++++++- 5 files changed, 22 insertions(+), 8 deletions(-) rename conf/{config.proto => config.defaults} (100%) [...]
Great! I wonder whether it would be easy to add tests for this new feature to our test suite... Merged into pu, thanks!
participants (3)
-
Eli Schwartz
-
Lukas Fleischer
-
Nodiv Byzero