fix(ls): let --domains answer nothing instead of raising

Reviewed-on: #15

README.md

@@ -289,6 +289,8 @@ CONFIG-SETS:
     bbb: true
 ```
 
+This is for bundles of per-app configuration that an operator turns on deliberately, a feature for instance. See [examples/config-sets.yml](./examples/config-sets.yml).
+
 **By itself**, when it carries `apply-when` and every app named there is part of the instance:
 
 ```yaml

alakazam.py

@@ -60,7 +60,7 @@ SETTINGS_PATH = ""  # path to the alakazam settings file
 GROUP_PATH = None  # path this run was invoked with, either an instance file or a group directory
 HIDE_SECRETS = False  # keep secret values out of the output, for runs whose logs are archived
 ABRA_DIR = None  # path to the abra data directory
-FETCH_MAX_AGE = 600 # seconds; recipe repos are refetched at most once per hour
+FETCH_MAX_AGE = 3600 # seconds; recipe repos are refetched at most once per hour
 ABRA_RETRIES = 6 # number of attempts per abra command that failed to reach the server
 ABRA_RETRY_DELAY = 2 # seconds before the first retry, doubled for every further one
 # lower case markers in the abra output that indicate an unreachable server instead of a failed operation
@@ -183,7 +183,7 @@ def get_relevant_config_paths(root_path: Path, group_path: Path, exclude_paths:
     """
     Collects every configuration file that can influence the result of this run: the 'alaka*.yml'
     inheritance chain from the root path down to the group path, the instance configurations below
-    it, the config sets of the root path and the connection configuration shipped with alakazam.
+    it, and every config-set file in the root path.
 
     Args:
         root_path (Path): The alakazam root path
@@ -482,21 +482,30 @@ def get_config_set_app_configs(
     return automatic_app_configs, 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]:
@@ -528,7 +537,7 @@ def merge_instance_configs(group_config: Dict[str, Any], instance_domain: str, i
     instance_apps = {k: v for k, v in instance_config.items() if k not in NON_APP_KEYS}
     automatic_app_configs, config_set_app_configs = get_config_set_app_configs(active_config_sets, config_sets, instance_apps)
     # Warn about config-set entries that target apps not listed in the instance config.
-    for config_set_app in set(config_set_app_configs) - set(instance_apps):
+    for config_set_app in (set(automatic_app_configs) | set(config_set_app_configs)) - set(instance_apps):
         logging.warning(
             f"Config-set specifies config for '{config_set_app}' but '{config_set_app}' is not listed "
             f"in the instance config for '{instance_domain}'. Skipping."
@@ -916,6 +925,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.
@@ -930,12 +967,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]:
@@ -1422,22 +1467,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)
 
@@ -1445,22 +1493,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)
@@ -2089,7 +2141,8 @@ def ls(recipes: Tuple[str], deployed: bool, undeployed: bool, domains: bool, ser
     else:
         instance_apps = get_apps(recipes)
     if domains:
-        print(list(zip(*sum(instance_apps.values(), [])))[1])
+        if apps_domains := sum(instance_apps.values(), []):
+            print(list(zip(*apps_domains))[1])
     else:
         print_all_apps(instance_apps)
 

requirements.txt

@@ -7,4 +7,4 @@ python-dotenv==1.0.0
 icecream==2.1.3
 packaging==24.0 
 GitPython==3.1.43
-uptime_kuma_api==1.2.1
+uptime-kuma-api2==2.7.0

tests/test_config_sets.py

@@ -120,6 +120,31 @@ class TestIntegrationReachesTheMergedConfig:
         assert "uncomment" not in self.merge(["authentik"])["authentik"]
 
 
+class TestUnknownAppKeys:
+    """A misspelt app key drops its configuration, which is only visible if it is reported."""
+
+    def test_a_typo_in_an_automatic_set_is_reported(self, caplog):
+        sets = {"integrate": {"apply-when": ["authentik", "nextcloud"],
+                              "nextcoud": {"env": {"A": "b"}}}}
+        with caplog.at_level(logging.WARNING):
+            merge_instance_configs({}, "example.com", {"authentik": None, "nextcloud": None}, sets)
+        assert "'nextcoud' is not listed" in caplog.text
+
+    def test_a_typo_in_an_explicit_set_is_reported(self, caplog):
+        sets = {"bbb": {"nextcoud": {"env": {"A": "b"}}}}
+        with caplog.at_level(logging.WARNING):
+            merge_instance_configs({}, "example.com",
+                                   {"nextcloud": None, "CONFIG-SETS": {"bbb": True}}, sets)
+        assert "'nextcoud' is not listed" in caplog.text
+
+    def test_a_correct_app_key_is_not_reported(self, caplog):
+        sets = {"integrate": {"apply-when": ["authentik", "nextcloud"],
+                              "nextcloud": {"env": {"A": "b"}}}}
+        with caplog.at_level(logging.WARNING):
+            merge_instance_configs({}, "example.com", {"authentik": None, "nextcloud": None}, sets)
+        assert caplog.text == ""
+
+
 class TestPrecedence:
     """An integration is the lowest layer, as combine.yml was before it."""
 

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: <reason>' 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

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'"]

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: <reason>' 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"