From 2663c0a499152a332726798969843b004ce2621b Mon Sep 17 00:00:00 2001
From: Valentin Lorentz <vlorentz@softwareheritage.org>
Date: Wed, 10 May 2023 11:23:13 +0200
Subject: [PATCH] config: Remove confusing magic in path handling
1. When passed a .yaml path (or any path with a non-whitelisted extension),
don't read .yml instead when the .yaml exists.
It's an extremely surprising behavior.
2. If the .yaml file does not exist, it will still try alternative extensions
in order not to break existing deployments which may rely on it, but it
raises a warning now.
3. When given a non-existing path, show an error log, but keep parsing
it as an empty config, in order not to break existing deployments.
---
swh/core/config.py | 43 ++++++++++++++++++-----------------
swh/core/tests/test_config.py | 13 ++++++++++-
2 files changed, 34 insertions(+), 22 deletions(-)
diff --git a/swh/core/config.py b/swh/core/config.py
index a5374718..a5f00a23 100644
--- a/swh/core/config.py
+++ b/swh/core/config.py
@@ -70,34 +70,35 @@ def exists_accessible(filepath: str) -> bool:
raise PermissionError("Permission denied: {filepath!r}")
-def config_basepath(config_path: str) -> str:
- """Return the base path of a configuration file"""
- if config_path.endswith(".yml"):
- return config_path[:-4]
-
- return config_path
-
-
def read_raw_config(base_config_path: str) -> Dict[str, Any]:
"""Read the raw config corresponding to base_config_path.
Can read yml files.
"""
- yml_file = f"{base_config_path}.yml"
- if exists_accessible(yml_file):
+ yml_file = config_path(base_config_path)
+ if yml_file is None:
+ logging.error("Config file %s does not exist, ignoring it.", base_config_path)
+ return {}
+ else:
logger.debug("Loading config file %s", yml_file)
with open(yml_file) as f:
return yaml.safe_load(f)
- return {}
-
-def config_exists(config_path):
+def config_path(config_path):
"""Check whether the given config exists"""
- basepath = config_basepath(config_path)
- return any(
- exists_accessible(basepath + extension) for extension in SWH_CONFIG_EXTENSIONS
- )
+ if exists_accessible(config_path):
+ return config_path
+ for extension in SWH_CONFIG_EXTENSIONS:
+ if exists_accessible(config_path + extension):
+ logger.warning(
+ "%s does not exist, using %s instead",
+ config_path,
+ config_path + extension,
+ )
+ return config_path + extension
+
+ return None
def read(
@@ -122,7 +123,7 @@ def read(
conf: Dict[str, Any] = {}
if conf_file:
- base_config_path = config_basepath(os.path.expanduser(conf_file))
+ base_config_path = os.path.expanduser(conf_file)
conf = read_raw_config(base_config_path) or {}
if not default_conf:
@@ -152,8 +153,8 @@ def priority_read(
# Try all the files in order
for filename in conf_filenames:
- full_filename = os.path.expanduser(filename)
- if config_exists(full_filename):
+ full_filename = config_path(os.path.expanduser(filename))
+ if full_filename is not None:
return read(full_filename, default_conf)
# Else, return the default configuration
@@ -303,6 +304,6 @@ def load_from_envvar(default_config: Optional[Dict[str, Any]] = None) -> Dict[st
), "SWH_CONFIG_FILENAME environment variable is undefined."
cfg_path = os.environ["SWH_CONFIG_FILENAME"]
- cfg = read_raw_config(config_basepath(cfg_path))
+ cfg = read_raw_config(cfg_path)
cfg = merge_configs(default_config or {}, cfg)
return cfg
diff --git a/swh/core/tests/test_config.py b/swh/core/tests/test_config.py
index 5f5005d8..fb4b7332 100644
--- a/swh/core/tests/test_config.py
+++ b/swh/core/tests/test_config.py
@@ -1,9 +1,10 @@
-# Copyright (C) 2015-2020 The Software Heritage developers
+# Copyright (C) 2015-2023 The Software Heritage developers
# See the AUTHORS file at the top-level directory of this distribution
# License: GNU General Public License version 3, or any later version
# See top-level LICENSE file for more information
import os
+from pathlib import Path
import shutil
import pkg_resources.extern.packaging.version
@@ -115,6 +116,16 @@ def test_read(swh_config):
assert res == parsed_conffile
+def test_read_dotyaml(swh_config):
+ swh_config2 = Path(str(swh_config).replace(".yml", ".yaml"))
+ swh_config.rename(swh_config2)
+ # when
+ res = config.read(swh_config2, default_conf)
+
+ # then
+ assert res == parsed_conffile
+
+
def test_read_no_default_conf(swh_config):
"""If no default config if provided to read, this should directly parse the config file
yaml
--
GitLab