From ba23f2c8e01d19977772ac9690b68e4ee73d8a4d Mon Sep 17 00:00:00 2001 From: Danny Groenewegen Date: Fri, 11 Sep 2026 16:14:32 +0200 Subject: [PATCH 1/2] fix(env): correctly set self referencing env values dotenv.set_key() rewrites every line sharing a key, breaking recipes that legitimately repeat a key (e.g. COMPOSE_FILE) to accumulate a value via bash expansion. Values that reference their own key (e.g. "$COMPOSE_FILE:extra.yml") are now routed to a new set_extending_key(), which only touches the one matching line by uncommenting or appending it, leaving every other line sharing that key untouched. --- alakazam.py | 67 +++++++++++++++++++++++++++------- tests/test_env_updates.py | 76 +++++++++++++++++++++++++++++++++++++++ tests/test_retry.py | 22 ++---------- 3 files changed, 133 insertions(+), 32 deletions(-) create mode 100644 tests/test_env_updates.py diff --git a/alakazam.py b/alakazam.py index 8aa7959..0674f18 100755 --- a/alakazam.py +++ b/alakazam.py @@ -894,6 +894,34 @@ def new_app(recipe: str, domain: str, server: str, version: str) -> None: logging.info(f'{recipe} created on {server} at {domain}') +def set_extending_key(path: Path, key: str, value: str) -> None: + """ + Sets a "key=value" line in the .env file at path, for keys that appear more than + once in the file (e.g. COMPOSE_FILE lines, which accumulate via bash expansion). + Unlike dotenv.set_key(), which would overwrite every line sharing that key. + - if that exact line already exists uncommented, nothing to do. + - if it exists but commented, uncomment it in place. + - otherwise, append it as a new line at the end of the file. + + Args: + path (str): Path to the .env configuration file to update. + key (str): The env var key. + value (str): The value the key should be set to. + """ + targetline = f'{key}="{value}"' + with open(path, "r") as file: + lines = file.readlines() + for i, line in enumerate(lines): + if line.lstrip("#").strip() == targetline: + if line.lstrip().startswith("#"): + lines[i] = f"{targetline}\n" + with open(path, "w") as file: + file.writelines(lines) + return + with open(path, "a") as file: + file.write(f"{targetline}\n") + + def update_configs(path: Path, config: Dict[str, Any]) -> None: """ Update the .env configuration files at the specified path according to the provided configuration dictionary. @@ -908,12 +936,20 @@ def update_configs(path: Path, config: Dict[str, Any]) -> None: if comment_keys := config.get("comment"): comment(comment_keys, path, True) if envs := config.get("env"): - uncomment(envs.keys(), path) + # A value that references its own key via shell expansion (e.g. + # "$COMPOSE_FILE:compose.mariadb.yml") is meant to extend an existing accumulation of + # values, not replace it. Those keys must be kept out of + # uncomment()/set_key(), which would otherwise affect every line sharing the key. + extending_keys = {key for key, value in envs.items() if f"${key}" in str(value)} + uncomment(envs.keys() - extending_keys, path) for key, value in envs.items(): logging.debug(f'set {key}={value} in {path}') if isinstance(value, dict): value=json.dumps(value) - dotenv.set_key(path, key, value, quote_mode="never") + if key in extending_keys: + set_extending_key(path, key, value) + else: + dotenv.set_key(path, key, value, quote_mode="never") def get_missing_secrets(domain: str) -> List[str]: @@ -1400,22 +1436,25 @@ def update_secret(domain: str, secret_name: str, secret: str, was_deployed: bool def uncomment(keys: List[str], path: str, match_all: bool = False) -> None: """ Uncomments lines in a configuration file that contain specified keys. - If 'match_all' is True, it matches against the entire line, otherwise, it matches only against the key. + If 'match_all' is True, it matches against the entire line (substring match), otherwise, it + matches only against the key exactly. Args: keys (list of str): The keys corresponding to the lines to be uncommented. path (str): Path to the file where lines will be uncommented. - match_all (bool): Whether to match the keys against the entire line or just the beginning. + match_all (bool): Whether to match the keys against the entire line or just the key. """ logging.debug(f'Uncomment {keys} in {path}') with open(path, "r") as file: lines = file.readlines() with open(path, "w") as file: for line in lines: - line_match = line.split("=")[0] # Match only keys if match_all: - line_match = line - if ('=' in line) and any(key in line_match for key in keys): + matched = ('=' in line) and any(key in line for key in keys) + else: + line_key = line.lstrip("#").split("=", 1)[0].strip() # Match only keys + matched = ('=' in line) and (line_key in keys) + if matched: line = line.lstrip("#").lstrip() file.write(line) @@ -1423,22 +1462,26 @@ def uncomment(keys: List[str], path: str, match_all: bool = False) -> None: def comment(keys: List[str], path: str, match_all: bool = False) -> None: """ Comments lines in a configuration file that contain specified keys. - If 'match_all' is True, it matches against the entire line, otherwise, it matches only against the key. + If 'match_all' is True, it matches against the entire line (substring match), otherwise, it + matches only against the key, exactly (so a key like "COMPOSE" doesn't also match a line whose + key is "COMPOSE_FILE"). Args: keys (list of str): The keys corresponding to the lines to be commented. path (str): Path to the file where lines will be commented. - match_all (bool): Whether to match the keys against the entire line or just the beginning. + match_all (bool): Whether to match the keys against the entire line or just the key. """ logging.debug(f'Comment {keys} in {path}') with open(path, "r") as file: lines = file.readlines() with open(path, "w") as file: for line in lines: - line_match = line.split("=")[0] # Match only keys if match_all: - line_match = line - if any(key in line_match for key in keys): + matched = any(key in line for key in keys) + else: + line_key = line.lstrip("#").split("=", 1)[0].strip() # Match only keys + matched = line_key in keys + if matched: line = line.lstrip("#").lstrip() line = f"#{line}" file.write(line) diff --git a/tests/test_env_updates.py b/tests/test_env_updates.py new file mode 100644 index 0000000..8b85241 --- /dev/null +++ b/tests/test_env_updates.py @@ -0,0 +1,76 @@ +"""Tests for how update_configs() applies a config's env/comment/uncomment keys to a .env file.""" + +import os +import sys + +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) + +from alakazam import update_configs + + +class TestUnknownConfigKeys: + def test_an_unknown_key_never_reaches_the_env_file(self, tmp_path): + """A promotion mechanism sets 'hold: ' on a recipe, which must pass through untouched.""" + env = tmp_path / "app.env" + env.write_text("#SMTP_HOST=mail.example.com\n") + update_configs(env, {"hold": "waiting for the upstream fix", "env": {"SMTP_HOST": "mail.a.org"}}) + content = env.read_text() + assert "SMTP_HOST=mail.a.org" in content + assert "hold" not in content + assert "waiting for the upstream fix" not in content + + def test_an_unknown_key_alone_changes_nothing(self, tmp_path): + env = tmp_path / "app.env" + env.write_text("#SMTP_HOST=mail.example.com\n") + update_configs(env, {"hold": "waiting for the upstream fix"}) + assert env.read_text() == "#SMTP_HOST=mail.example.com\n" + + +class TestExtendingEnvKeys: + """ + Some env keys are repeated on multiple lines, relying on bash accumulation + ("COMPOSE_FILE=\"$COMPOSE_FILE:compose.mariadb.yml\"") to build up a value. + A config that sets such a key should extend that accumulation, not replace + or uncomment every line sharing the key. + """ + + ACCUMULATING_ENV = ( + 'COMPOSE_FILE="compose.yml"\n' + 'COMPOSE_FILE="$COMPOSE_FILE:compose.mariadb.yml"\n' + '# COMPOSE_FILE="$COMPOSE_FILE:compose.onlyoffice.yml"\n' + ) + + def test_a_key_sharing_a_prefix_is_not_matched_by_substring(self, tmp_path): + """COMPOSE must not be treated as matching lines whose key is COMPOSE_FILE.""" + env = tmp_path / "app.env" + env.write_text(self.ACCUMULATING_ENV) + update_configs(env, {"env": {"COMPOSE": "compose.custom.yml"}}) # not self-referencing + lines = env.read_text().splitlines() + assert lines[:-1] == self.ACCUMULATING_ENV.splitlines() # every original line untouched + assert lines[-1] == 'COMPOSE=compose.custom.yml' # set as its own, unrelated key + + def test_a_self_referencing_value_is_appended_without_touching_other_lines(self, tmp_path): + env = tmp_path / "app.env" + env.write_text(self.ACCUMULATING_ENV) + update_configs(env, {"env": {"COMPOSE_FILE": "$COMPOSE_FILE:../../customoverride.yml"}}) + lines = env.read_text().splitlines() + assert lines[:-1] == self.ACCUMULATING_ENV.splitlines() # every original line untouched + assert lines[-1] == 'COMPOSE_FILE="$COMPOSE_FILE:../../customoverride.yml"' # appended + + def test_setting_the_same_self_referencing_value_twice_is_idempotent(self, tmp_path): + env = tmp_path / "app.env" + env.write_text(self.ACCUMULATING_ENV) + config = {"env": {"COMPOSE_FILE": "$COMPOSE_FILE:../../customoverride.yml"}} + update_configs(env, config) + update_configs(env, config) + lines = env.read_text().splitlines() + assert lines.count('COMPOSE_FILE="$COMPOSE_FILE:../../customoverride.yml"') == 1 + + def test_a_self_referencing_value_matching_an_existing_commented_line_uncomments_it_in_place(self, tmp_path): + env = tmp_path / "app.env" + env.write_text(self.ACCUMULATING_ENV) + update_configs(env, {"env": {"COMPOSE_FILE": "$COMPOSE_FILE:compose.onlyoffice.yml"}}) + lines = env.read_text().splitlines() + assert lines[:-1] == self.ACCUMULATING_ENV.splitlines()[:-1] # untouched, incl. the already-active line + assert lines[-1] == 'COMPOSE_FILE="$COMPOSE_FILE:compose.onlyoffice.yml"' # uncommented in place + assert len(lines) == len(self.ACCUMULATING_ENV.splitlines()) # no new line appended diff --git a/tests/test_retry.py b/tests/test_retry.py index e62e93d..3488913 100644 --- a/tests/test_retry.py +++ b/tests/test_retry.py @@ -1,4 +1,4 @@ -"""Tests for which abra failures may be retried, and which configuration keys are ignored.""" +"""Tests for which abra failures may be retried.""" import os import sys @@ -9,7 +9,7 @@ import pytest sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) import alakazam -from alakazam import abra, is_connection_error, update_configs +from alakazam import abra, is_connection_error class FakeProcess: @@ -83,21 +83,3 @@ class TestConnectionErrors: ]) def test_operational_failures_are_not_retried(self, output): assert not is_connection_error(output) - - -class TestUnknownConfigKeys: - def test_an_unknown_key_never_reaches_the_env_file(self, tmp_path): - """A promotion mechanism sets 'hold: ' on a recipe, which must pass through untouched.""" - env = tmp_path / "app.env" - env.write_text("#SMTP_HOST=mail.example.com\n") - update_configs(env, {"hold": "waiting for the upstream fix", "env": {"SMTP_HOST": "mail.a.org"}}) - content = env.read_text() - assert "SMTP_HOST=mail.a.org" in content - assert "hold" not in content - assert "waiting for the upstream fix" not in content - - def test_an_unknown_key_alone_changes_nothing(self, tmp_path): - env = tmp_path / "app.env" - env.write_text("#SMTP_HOST=mail.example.com\n") - update_configs(env, {"hold": "waiting for the upstream fix"}) - assert env.read_text() == "#SMTP_HOST=mail.example.com\n" -- 2.54.0 From 17abd1f02bd4413b3f9028636094e1f4c4f78fe9 Mon Sep 17 00:00:00 2001 From: Danny Groenewegen Date: Fri, 11 Sep 2026 17:03:16 +0200 Subject: [PATCH 2/2] fix(jinja): recurse into lists when substituting template variables substitute_jinja_variable() only recursed into dict values; a list value fell into the scalar branch, where str(value) stringified the whole list --- alakazam.py | 31 +++++++++++++++++++----------- tests/test_jinja_substitution.py | 33 ++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 11 deletions(-) create mode 100644 tests/test_jinja_substitution.py diff --git a/alakazam.py b/alakazam.py index 0674f18..105a71d 100755 --- a/alakazam.py +++ b/alakazam.py @@ -431,21 +431,30 @@ def get_config_set_app_configs( return config_set_app_configs -def substitute_jinja_variable(jinja_dict, subs_dict) -> None: +def substitute_jinja_variable(node, subs_dict): """ - This function recursively traverses the given jinja_dict and wherever it finds a jinja template variable, it replaces it with the corresponding value from the subs_dict. + Recursively substitutes jinja template variables inside node with values from subs_dict. + Dicts and lists are walked and updated in place. The rendered value is also returned so a + recursive call can assign a replaced scalar. Args: - jinja_dict (dict): The dictionary which may contain jinja template variables. Can be a nested dictionary. - subs_dict (dict): The dictionary containing the substitutions for the jinja template variables. + node: A dict, list, or scalar which may contain jinja template variables, at any nesting. + subs_dict (dict): The dictionary containing the substitutions for the jinja template variables. + + Returns: + The same dict/list (mutated in place), or the rendered scalar. """ - for key, value in jinja_dict.items(): - if isinstance(value, dict): # If value itself is dictionary - substitute_jinja_variable(value, subs_dict) # Recursive call - else: - if "{{" in str(value) and "}}" in str(value): # If value is a jinja template - template = Template(str(value)) - jinja_dict[key] = template.render(subs_dict) + if isinstance(node, dict): + items = node.items() + elif isinstance(node, list): + items = enumerate(node) + else: + if "{{" in str(node) and "}}" in str(node): # If node is a jinja template + return Template(str(node)).render(subs_dict) + return node + for key, item in items: + node[key] = substitute_jinja_variable(item, subs_dict) + return node def merge_instance_configs(group_config: Dict[str, Any], instance_domain: str, instance_config: Dict[str, Any], config_sets: Optional[Dict[str, Any]] = None) -> Dict[str, Any]: diff --git a/tests/test_jinja_substitution.py b/tests/test_jinja_substitution.py new file mode 100644 index 0000000..897f8c2 --- /dev/null +++ b/tests/test_jinja_substitution.py @@ -0,0 +1,33 @@ +"""Tests for substitute_jinja_variable()'s recursion into dicts, lists, and scalars.""" + +import os +import sys + +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) + +from alakazam import substitute_jinja_variable + + +class TestSubstituteJinjaVariable: + def test_a_scalar_in_a_dict_substitutes(self): + config = {"nextcloud": {"env": {"DOMAIN": "{{ domain }}"}}} + substitute_jinja_variable(config, {"domain": "nc.example.com"}) + assert config["nextcloud"]["env"]["DOMAIN"] == "nc.example.com" + + def test_a_scalar_in_a_list_substitutes(self): + config = {"nextcloud": {"initial-hooks": [ + "app install_apps", + "app set_app_config files default_quota {{ quota }}", + "app run_occ", + ]}} + substitute_jinja_variable(config, {"quota": "0"}) + hooks = config["nextcloud"]["initial-hooks"] + assert isinstance(hooks, list) and len(hooks) == 3 + assert hooks[0] == "app install_apps" + assert "0" in hooks[1] + assert hooks[2] == "app run_occ" + + def test_a_scalar_in_a_list_without_a_template_is_left_untouched(self): + config = {"nextcloud": {"initial-hooks": ["app run_occ 'app:install groupfolders'"]}} + substitute_jinja_variable(config, {"quota": "0"}) + assert config["nextcloud"]["initial-hooks"] == ["app run_occ 'app:install groupfolders'"] -- 2.54.0