fix(secrets): read a secret without asking for a terminal

Reviewed-on: #15

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
@@ -79,6 +79,8 @@ CONNECTION_ERRORS = (
 MISSING_SECRET_ERROR = "doesn't exist on server"
 # abra subcommands that take a secret as an argument or return one
 SECRET_COMMANDS = ("insert", "generate")
+# marker in the abra output for an app that is not running, which is not an error to its callers
+NOT_DEPLOYED_ERROR = "is not deployed?"
 # markers in the abra output for a secret run that had nothing left to do, which abra exits 1 on
 NO_SECRETS_GENERATED = "no secrets generated"
 NO_SECRETS_TO_REMOVE = "no secrets to remove"
@@ -88,6 +90,7 @@ PENDING_STATUS_RE = re.compile(r".*(starting|unknown|unhealthy).*")
 INIT_CONTAINER_MARKER = "-init"
 PS_WAIT_INTERVAL = 10 # seconds between two health checks of 'ps --wait'
 PS_WAIT_TIMEOUT = 900 # seconds 'ps --wait' waits for all services to become healthy
+PS_WAIT_SETTLE = 30 # seconds before a health check, so that replaced containers are gone
 # defaults of a readiness hook, each of them overridable per hook in the app configuration
 READINESS_RETRIES = 20 # attempts before a readiness hook is considered failed
 READINESS_INTERVAL = 60 # seconds between two attempts
@@ -482,21 +485,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]:
@@ -772,11 +784,30 @@ def abra(*args: str, machine_output: bool = False, ignore_error: bool = False, s
             f'{" ".join(command)} \n STDOUT: \n {process.stdout.decode()} \n STDERR: {process.stderr.decode()}')
     if machine_output:
         if output:= process.stdout.decode():
-            return json.loads(output)
+            return parse_machine_output(output)
         else:
             return {}
     return process.stdout.decode()
 
+
+def parse_machine_output(output: str) -> Any:
+    """
+    Parses the json abra writes with '-m', which some commands spread over one document per line.
+
+    'recipe versions' prints a document per recipe version instead of one for the whole answer, so
+    reading the output as a single document stops after the first line with "Extra data".
+
+    Args:
+        output (str): The stdout of an abra call that asked for machine output
+
+    Returns:
+        Any: The document abra wrote, or the list of them when it wrote more than one
+    """
+    try:
+        return json.loads(output)
+    except json.JSONDecodeError:
+        return [json.loads(line) for line in output.splitlines() if line.strip()]
+
 def sync_repo(path: Path) -> None:
     """
     Fetches a git repository and fast-forwards it, mirroring what abra does when it is online.
@@ -916,6 +947,71 @@ def new_app(recipe: str, domain: str, server: str, version: str) -> None:
     logging.info(f'{recipe} created on {server} at {domain}')
 
 
+def format_env_value(value: Any, default_quote: str = '') -> Any:
+    """
+    Renders a configuration value as the text that follows "KEY=" in an .env file.
+
+    A value keeps the quotes of its YAML scalar, because abra expands a "$" inside double quotes and
+    reads single quotes literally. Quotes of the same style within the value, which jinja can bring
+    in through a global, would end the line early. The other style is used where it carries the same
+    meaning, which needs no escape and is understood by every reader of the file. Only a value that
+    relies on expansion or on escapes of its own keeps its style and has its quotes escaped.
+
+    Args:
+        value (Any): The value to render, with or without the quotes of its YAML scalar
+        default_quote (str): Quote style to wrap a value that brings none of its own
+
+    Returns:
+        Any: The value as it can be written after "KEY=", non-strings unchanged
+    """
+    if not isinstance(value, str):
+        return value
+    quote = value[0] if len(value) > 1 and value[0] == value[-1] and value[0] in '"\'' else ''
+    inner = value[1:-1] if quote else value
+    quote = quote or default_quote
+    if not quote or quote not in inner:
+        return f'{quote}{inner}{quote}' if quote else inner
+    # "$" expands only inside double quotes and a backslash only escapes there, so a value holding
+    # either means what its own style says and cannot be moved to the other one
+    other = '"' if quote == "'" else "'"
+    if other not in inner and '$' not in inner and '\\' not in inner:
+        return f'{other}{inner}{other}'
+    if quote == "'":
+        logging.warning(f"the value {value} holds a single quote inside single quotes, which ends "
+                        "it early when it is read back, write it in double quotes instead")
+        return f"'{inner}'"
+    # the already escaped quotes are normalised first, so that they are not escaped a second time
+    return '"{}"'.format(inner.replace('\\"', '"').replace('"', '\\"'))
+
+
+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 = '{}={}'.format(key, format_env_value(value, default_quote='"'))
+    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 +1026,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, format_env_value(value), quote_mode="never")
 
 
 def get_missing_secrets(domain: str) -> List[str]:
@@ -1107,18 +1211,8 @@ def share_secrets(app1_domain: str, app2_domain: str, secrets: Dict[str, str], s
     app1_stored_secrets = {x['name']: str2bool(x['created on server']) for x in app1_stored_secrets}
     app2_stored_secrets = abra("app", "secret", "ls", app2_domain, machine_output=True)
     app2_stored_secrets = {x['name']: str2bool(x['created on server']) for x in app2_stored_secrets}
-    try:
-        app1_container = abra("app" ,"ps", "-m", app1_domain, machine_output=True)
-        app1_container = list(app1_container)
-    except RuntimeError:
-        # App1 is not deployed
-        app1_container = []
-    try:
-        app2_container = abra("app" ,"ps", "-m", app2_domain, machine_output=True)
-        app2_container = list(app2_container)
-    except RuntimeError:
-        # App2 is not deployed
-        app2_container = []
+    app1_container = get_running_containers(app1_domain)
+    app2_container = get_running_containers(app2_domain)
     for app2_secret in secrets:
         app1_secret = secrets[app2_secret]
         try:
@@ -1190,6 +1284,31 @@ def share_secrets(app1_domain: str, app2_domain: str, secrets: Dict[str, str], s
                 )
 
 
+def get_running_containers(domain: str) -> List:
+    """
+    Lists the containers of an app, treating an app that is not deployed as having none.
+
+    Every other failure is raised: reading it as "not running" would turn a server that cannot be
+    reached into a silently skipped secret exchange.
+
+    Args:
+        domain (str): The app domain to read the containers of
+
+    Returns:
+        list: The containers abra reports, empty when the app is not deployed
+
+    Raises:
+        RuntimeError: If abra fails for any other reason
+    """
+    try:
+        return list(abra("app", "ps", "-m", domain, machine_output=True))
+    except RuntimeError as error:
+        if NOT_DEPLOYED_ERROR not in str(error):
+            raise
+        logging.info(f"{domain} is not deployed, it holds no containers to read a secret from")
+        return []
+
+
 def get_secret(domain: str, secret_name: str, containers: List) -> str:
     """
     Retrieves a specified secret from the application specified by its domain using the 'abra' command.
@@ -1204,7 +1323,9 @@ def get_secret(domain: str, secret_name: str, containers: List) -> str:
     """
     for container in containers:
         try:
-            secret = abra("app", "run", domain, container, "cat", f"/var/run/secrets/{secret_name}")
+            # -t is --no-tty: a run that asks for a terminal has none in ci, and a terminal would
+            # translate the line endings of the value on its way out
+            secret = abra("app", "run", "-t", domain, container, "cat", f"/var/run/secrets/{secret_name}")
             return str(secret)
         except RuntimeError:
             continue
@@ -1214,6 +1335,30 @@ def get_secret(domain: str, secret_name: str, containers: List) -> str:
     return get_secret_from_host(domain, secret_name)
 
 
+def run_over_ssh(host: str, command: str, purpose: str) -> str:
+    """
+    Runs a command on a host over ssh and returns its output, refusing to read a failure as empty.
+
+    Args:
+        host (str): The host to run the command on
+        command (str): The shell command to run there
+        purpose (str): What the command is for, used in the error message
+
+    Returns:
+        str: The stdout of the command, unchanged
+
+    Raises:
+        RuntimeError: If ssh or the command itself fails
+    """
+    result = subprocess.run(["ssh", host, command], capture_output=True, text=True)
+    if result.returncode:
+        raise RuntimeError(f"{purpose} on {host} exited with code {result.returncode}: "
+                           f"{result.stderr.strip() or 'no output'}")
+    if error := result.stderr.strip():
+        logging.warning(f"{purpose} on {host} wrote to stderr: {error}")
+    return result.stdout
+
+
 def get_secret_from_host(domain: str, secret_name: str) -> str:
     """
     Read a Docker Swarm secret value by SSHing to the host and reading from the
@@ -1241,8 +1386,7 @@ def get_secret_from_host(domain: str, secret_name: str) -> str:
         f"    '.[]? | .SecretID + \" \" + $cid + \" \" + .SecretName'; "
         f"done | grep ' {stack_name}_{secret_name}_' | head -1"
     )
-    result = subprocess.run(["ssh", host, find_cmd], capture_output=True, text=True)
-    match_line = result.stdout.strip()
+    match_line = run_over_ssh(host, find_cmd, f"the lookup of secret '{secret_name}'").strip()
     if not match_line:
         raise RuntimeError(f"Secret '{secret_name}' not found in stack '{stack_name}' on {host}")
 
@@ -1254,8 +1398,8 @@ def get_secret_from_host(domain: str, secret_name: str) -> str:
         f"cat /var/lib/docker/containers/{container_id}/mounts/secrets/{secret_id} 2>/dev/null "
         f"|| sudo cat /var/lib/docker/containers/{container_id}/mounts/secrets/{secret_id}"
     )
-    read_result = subprocess.run(["ssh", host, read_cmd], capture_output=True, text=True)
-    value = read_result.stdout  # no strip — trailing newline could be part of the secret
+    # no strip — a trailing newline could be part of the secret
+    value = run_over_ssh(host, read_cmd, f"the read of secret '{secret_name}'")
     if not value:
         raise RuntimeError(f"Could not read value for secret '{secret_name}' on {host}")
     return value
@@ -1422,22 +1566,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 +1592,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)
@@ -1833,8 +1984,10 @@ def get_deployed_apps(apps: Tuple[str]) -> Dict[str, str]:
 @click.option('-e', '--execute-hooks', is_flag=True, help='run post-deployment commands.')
 @click.option('-f', '--force', is_flag=True, help='force redeployment even if the application is already deployed.')
 @click.option('-c', '--converge-checks', is_flag=True, help='perform convergence checks during deployment.')
+@click.option('--timeout', default=PS_WAIT_TIMEOUT, show_default=True, metavar='<Seconds>', help='how long to wait for all services to become healthy.')
+@click.option('--settle', default=PS_WAIT_SETTLE, show_default=True, metavar='<Seconds>', help='how long to give the replaced services to be taken down before their health is read.')
 @click.option('noninteractive', '-n', '--non-interactive', is_flag=True, help='Run this command non-interactively')
-def deploy(recipes: Tuple[str], execute_hooks: bool, force: bool, converge_checks: bool, noninteractive: bool) -> None:
+def deploy(recipes: Tuple[str], execute_hooks: bool, force: bool, converge_checks: bool, timeout: int, settle: int, noninteractive: bool) -> None:
     """
     Deploys applications as specified in the configuration.
     """
@@ -1845,19 +1998,33 @@ def deploy(recipes: Tuple[str], execute_hooks: bool, force: bool, converge_check
     print_all_apps(instance_apps)
     if not instance_apps or not (noninteractive or input(f"Do you really want to deploy these apps? Type YES: ") == "YES"):
         return
-    deploy_apps(instance_apps, execute_hooks=execute_hooks, force=force, converge_checks=converge_checks)
+    deploy_apps(instance_apps, execute_hooks=execute_hooks, force=force, converge_checks=converge_checks,
+                timeout=timeout, settle=settle)
 
 
-def deploy_apps(instance_apps: Dict[str, List[List]], execute_hooks: bool = False, force: bool = False, converge_checks: bool = False) -> None:
+def deploy_apps(instance_apps: Dict[str, List[List]], execute_hooks: bool = False, force: bool = False,
+                converge_checks: bool = False, wait: bool = True, timeout: int = PS_WAIT_TIMEOUT,
+                settle: int = PS_WAIT_SETTLE) -> None:
     """
     Deploys the given apps with the version taken from their configuration.
 
+    The apps do not wait for each other, so the run blocks until every service of all of them is
+    healthy, and only then are the deploy-hooks executed: a hook needs the app it addresses to be
+    up again. A caller that arranges the waiting itself passes wait=False.
+
     Args:
         instance_apps (dict): Instances as keys and lists of lists [app name, domain, ...] as values
         execute_hooks (bool): run the deploy-hooks from the configuration afterwards.
         force (bool): deploy even if the app is already deployed.
         converge_checks (bool): perform convergence checks during deployment.
+        wait (bool): block until every deployed service is healthy.
+        timeout (int): Seconds to wait for all services to become healthy.
+        settle (int): Seconds to give the services to be replaced before their health is read.
+
+    Raises:
+        click.ClickException: If a service is still unhealthy when the timeout is reached
     """
+    deployed = []
     for instance, apps_to_deploy in instance_apps.items():
         for app, domain, *_ in apps_to_deploy:
             app_config = get_value(INSTANCE_CONFIGS, instance, app)
@@ -1869,16 +2036,22 @@ def deploy_apps(instance_apps: Dict[str, List[List]], execute_hooks: bool = Fals
                 cmd.append("--chaos")
             if force:
                 cmd.append("--force")
-            if not execute_hooks and not converge_checks:
+            if not converge_checks:
                 cmd.append("--no-converge-checks")
             cmd.append(domain)
             if version not in ['latest', 'chaos']:
                 cmd.append(version)
             print(f'deploy {domain} with version "{version}"')
             print(abra("app", *cmd))
-            if execute_hooks:
-                logging.info(f'execute commands for {domain}')
-                execute_cmds(app_config, deploy=True, instance_domain=instance)
+            deployed.append((instance, app, domain, app_config))
+    if not deployed:
+        return
+    if wait:
+        wait_for_healthy_apps(tuple({app for _, app, *_ in deployed}), timeout, settle=settle)
+    if execute_hooks:
+        for instance, app, domain, app_config in deployed:
+            logging.info(f'execute commands for {domain}')
+            execute_cmds(app_config, deploy=True, instance_domain=instance)
 
 
 @cli.command()
@@ -1889,12 +2062,15 @@ def deploy_apps(instance_apps: Dict[str, List[List]], execute_hooks: bool = Fals
 @click.option('--redeploy-wait', type=int, default=20, metavar='<SECONDS>',
               help='seconds to wait between undeploy and deploy in --redeploy mode (default: 20).')
 @click.option('-c', '--converge-checks', is_flag=True, help='perform convergence checks during deployment.')
+@click.option('--timeout', default=PS_WAIT_TIMEOUT, show_default=True, metavar='<Seconds>', help='how long to wait for all services to become healthy.')
+@click.option('--settle', default=PS_WAIT_SETTLE, show_default=True, metavar='<Seconds>', help='how long to give the replaced services to be taken down before their health is read.')
 @click.option('noninteractive', '-n', '--non-interactive', is_flag=True, help='Run this command non-interactively')
 def upgrade(recipes: Tuple[str], execute_hooks: bool, dry_run: bool, redeploy: bool,
-            converge_checks: bool, noninteractive: bool, redeploy_wait: int) -> None:
+            converge_checks: bool, timeout: int, settle: int, noninteractive: bool, redeploy_wait: int) -> None:
     """
     Upgrades specified applications by executing the upgrade commands via the 'abra' command-line interface.
     It checks the current deployment status of the apps and performs upgrades only where necessary, with options to execute additional commands or perform a dry run. It either took the target version from the configuration or it uses the latest available version.
+    The apps are upgraded without waiting for each other, then the run blocks until every service of them is healthy, and only then the upgrade hooks are executed.
     """
     deployed_instance_apps = get_apps_by_deployment(recipes, deployed=True)
     upgrade_cmds = []
@@ -1918,7 +2094,7 @@ def upgrade(recipes: Tuple[str], execute_hooks: bool, dry_run: bool, redeploy: b
             elif version_type == 'unknown':
                 logging.warning(f"Cold not detect deployed version of {domain}")
                 continue
-            if not execute_hooks and not converge_checks:
+            if not converge_checks:
                 upgrade_cmd.append("--no-converge-checks")
             upgrade_cmd.append(domain)
             deployed_version_type = classify_version(deployed_version)
@@ -1943,32 +2119,63 @@ def upgrade(recipes: Tuple[str], execute_hooks: bool, dry_run: bool, redeploy: b
                 if (not (note:= release_notes.get(app))) or len(note) <= len(release_note):
                     release_notes[app] = release_note
         deployed_instance_apps[instance] = upgrade_apps
-    print_all_apps(deployed_instance_apps)
+    print_all_apps(deployed_instance_apps, headers=("from", "to"))
     if not noninteractive:
         input("Press any key to show release notes.")
     for app, note in release_notes.items():
         print(app)
         print(note)
-    if not dry_run and noninteractive or input(f"Do you really want to upgrade these apps? Type YES: ") == "YES":
+    if dry_run:
+        if execute_hooks:
+            # shown instead of run, so that a dry run also answers what the -e would have done
+            for _, app_config, _, instance_domain in upgrade_cmds:
+                execute_cmds(app_config, upgrade=True, dry_run=True, instance_domain=instance_domain)
+        return
+    if noninteractive or input("Do you really want to upgrade these apps? Type YES: ") == "YES":
+        upgraded, failed = [], []
         for app, app_config, upgrade_cmd, instance_domain in upgrade_cmds:
             app_domain = app_config.get('app_domain')
-            # a recipe can bring a new secret along, which abra refuses to deploy without. The app
-            # is running, so nothing else can be missing: it had all of them when it was deployed
-            if missing := get_missing_secrets(app_domain):
-                print(f"{app_domain} is missing {len(missing)} secret(s): {', '.join(sorted(missing))}")
-                create_secrets((app,), instances=(instance_domain,))
-            if redeploy:
-                upgrade_cmd.pop(0)
-                upgrade_cmd.insert(0, 'deploy')
-                print(f'undeploy {app_domain}')
-                print(abra("app", "undeploy", '--no-input', app_domain))
-                print(f'waiting {redeploy_wait}s …')
-                sleep(redeploy_wait)
-            print(f'deploy {app_domain}')
-            print(abra("app", *upgrade_cmd))
-            if execute_hooks:
-                logging.info(f'execute commands for {app_domain}')
-                execute_cmds(app_config, upgrade=True, instance_domain=instance_domain)
+            try:
+                # a recipe can bring a new secret along, which abra refuses to deploy without. The
+                # app is running, so nothing else can be missing: it had all of them when deployed
+                if missing := get_missing_secrets(app_domain):
+                    print(f"{app_domain} is missing {len(missing)} secret(s): {', '.join(sorted(missing))}")
+                    create_secrets((app,), instances=(instance_domain,))
+                if redeploy:
+                    upgrade_cmd.pop(0)
+                    upgrade_cmd.insert(0, 'deploy')
+                    print(f'undeploy {app_domain}')
+                    print(abra("app", "undeploy", '--no-input', app_domain))
+                    print(f'waiting {redeploy_wait}s …')
+                    sleep(redeploy_wait)
+                print(f'deploy {app_domain}')
+                print(abra("app", *upgrade_cmd))
+            except RuntimeError as error:
+                # the apps behind this one do not depend on it, so they are still upgraded and the
+                # run ends on the failure instead of dropping everything where it stands
+                logging.error(f"upgrade of {app_domain} failed: {error}")
+                failed.append(app_domain)
+                continue
+            upgraded.append((app, app_config, instance_domain))
+        problems = []
+        if failed:
+            problems.append(
+                f"{len(failed)} of {len(upgrade_cmds)} app(s) failed to upgrade: {', '.join(failed)}")
+        if upgraded:
+            try:
+                # the deployments were started without waiting for each other, so the services are
+                # still settling here and a hook would reach a container that is not up again yet
+                wait_for_healthy_apps(tuple({app for app, *_ in upgraded}), timeout, settle=settle)
+            except click.ClickException as error:
+                # collected rather than raised, so that it cannot hide the apps that failed before
+                problems.append(error.message)
+            else:
+                if execute_hooks:
+                    for app, app_config, instance_domain in upgraded:
+                        logging.info(f"execute commands for {app_config.get('app_domain')}")
+                        execute_cmds(app_config, upgrade=True, instance_domain=instance_domain)
+        if problems:
+            raise click.ClickException("\n".join(problems))
 
 
 @cli.command()
@@ -2089,23 +2296,22 @@ 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)
 
 
-def print_all_apps(instance_apps: Dict[str, List[List]]) -> None:
+def print_all_apps(instance_apps: Dict[str, List[List]], headers: Tuple[str, str] = ("", "")) -> None:
     """
-    Prints a detailed list of all specified applications including their instances and domains. 
+    Prints one table per instance, listing its apps with their domain and whatever follows.
 
     Args:
-        apps (list): List of applications to print details for.
-
-    Returns:
-        dict: Returns a dictionary with application details grouped by instance.
+        instance_apps (dict): Instances as keys and lists of lists [app name, domain, ...] as values
+        headers (tuple): Titles for the two columns after the domain, unlabelled by default
     """
     for instance, apps in instance_apps.items():
-        print(tabulate(apps, headers=["",instance, "", ""], tablefmt='rounded_outline'))
+        print(tabulate(apps, headers=["", instance, *headers], tablefmt='rounded_outline'))
         print()
 
 def sort_apps_by_dependency(apps: List[str], instance_config: Dict[str, Any]) -> List[str]:
@@ -2286,8 +2492,13 @@ def get_latest_version(app: str) -> str:
     Returns:
         str: The latest version of the application as a string.
     """
-    versions = abra('recipe', 'versions', app, machine_output=True)
-    latest = sorted([(version.parse(v['version']), v['version']) for v in versions])[-1][1]
+    releases = abra('recipe', 'versions', app, machine_output=True)
+    # a release is the list of images of one recipe version, and each of them carries that version
+    images = [image for release in releases
+              for image in (release if isinstance(release, list) else [release])]
+    if not images:
+        raise click.ClickException(f"abra reports no versions for {app}, pin one in the configuration")
+    latest = sorted([(version.parse(i['version']), i['version']) for i in images])[-1][1]
     logging.debug(f'latest version of {app}: {latest}')
     return latest
 
@@ -2413,8 +2624,10 @@ def format_recipe_secrets(recipe_secrets: Dict[str, List[str]]) -> str:
 @click.option('-e', '--execute-hooks', is_flag=True, help='run post-deployment commands.')
 @click.option('-c', '--converge-checks', is_flag=True, help='perform convergence checks during deployment.')
 @click.option('only', '-o', '--only', multiple=True, metavar='<conf|secret-hooks|exchange|generate>', help='Only run the listed secret variants when creating the secrets anew, all of them by default. Comma separated or specified multiple times.')
+@click.option('--timeout', default=PS_WAIT_TIMEOUT, show_default=True, metavar='<Seconds>', help='how long to wait for all services to become healthy.')
+@click.option('--settle', default=PS_WAIT_SETTLE, show_default=True, metavar='<Seconds>', help='how long to give the replaced services to be taken down before their health is read.')
 @click.option('noninteractive', '-n', '--non-interactive', is_flag=True, help='Run this command non-interactively')
-def reinsert_secrets(secrets: Tuple[str], execute_hooks: bool, converge_checks: bool, only: Tuple[str], noninteractive: bool) -> None:
+def reinsert_secrets(secrets: Tuple[str], execute_hooks: bool, converge_checks: bool, only: Tuple[str], timeout: int, settle: int, noninteractive: bool) -> None:
     """
     Rotates secrets: undeploys the affected apps, removes the given secrets, creates them anew and redeploys the apps that were deployed before.
 
@@ -2430,7 +2643,8 @@ def reinsert_secrets(secrets: Tuple[str], execute_hooks: bool, converge_checks:
     undeploy_apps(deployed_apps)
     purge_app_secrets(recipe_secrets)
     create_secrets(recipes, variants=variants)
-    deploy_apps(deployed_apps, execute_hooks=execute_hooks, converge_checks=converge_checks)
+    deploy_apps(deployed_apps, execute_hooks=execute_hooks, converge_checks=converge_checks,
+                timeout=timeout, settle=settle)
 
 
 @cli.command()
@@ -2511,22 +2725,31 @@ def print_containers(containers: List[Tuple[str, Dict[str, Any]]], verbose: bool
     print(tabulate(rows, 'firstrow', 'rounded_outline'), flush=True)
 
 
-def wait_for_healthy_apps(recipes: Tuple[str], timeout: int, verbose: bool = False) -> None:
+def wait_for_healthy_apps(recipes: Tuple[str], timeout: int, verbose: bool = False, settle: int = 0) -> None:
     """
     Blocks until no service of the selected apps is starting, unknown or unhealthy any more.
 
-    The status is checked right away and then every PS_WAIT_INTERVAL seconds until the timeout
-    is reached. Having nothing deployed is a failure rather than an early success: waiting for
-    the health of an environment that does not exist would report a broken deployment as green.
+    The status is checked after the settle delay and then every PS_WAIT_INTERVAL seconds until the
+    timeout is reached. Having nothing deployed is a failure rather than an early success: waiting
+    for the health of an environment that does not exist would report a broken deployment as green.
+
+    The delay matters wherever apps are being replaced: such a service keeps reporting its old
+    container for a moment, and that one is healthy, it has been running all along. Reading the
+    health right away would pass on the deployment that is about to go away. An environment that is
+    built from scratch has nothing to replace and leaves the delay at zero.
 
     Args:
         recipes (tuple): Recipes to wait for, all of them if empty
         timeout (int): Seconds to wait before giving up
         verbose (bool): Add the chaos, state and image columns to the final table
+        settle (int): Seconds to give replaced services to be taken down before the first check
 
     Raises:
         click.ClickException: If nothing is deployed, or if services are still not healthy when the timeout is reached
     """
+    if settle:
+        print(f"waiting {settle}s for the replaced services to be taken down", flush=True)
+        sleep(settle)
     deadline = time() + timeout
     while True:
         instance_apps = get_apps_by_deployment(recipes, deployed=True)
@@ -2643,7 +2866,8 @@ def setup_environment(recipes: Tuple[str], converge_checks: bool = False, timeou
         for level in group_apps_by_dependency(list(domains), instance_config):
             create_secrets(tuple(level), instances=(instance,), strict=True,
                            variants={"secret-hooks", "exchange", "generate"})
-            deploy_apps({instance: [[app, domains[app]] for app in level]}, converge_checks=converge_checks)
+            deploy_apps({instance: [[app, domains[app]] for app in level]},
+                        converge_checks=converge_checks, wait=False)
             for app in level:
                 run_readiness_hooks(domains[app], instance_config[app])
     wait_for_healthy_apps(recipes, timeout)

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_deploy_hooks.py

@@ -0,0 +1,132 @@
+"""Tests for what a deploy run does: deploy everything, wait for all of it, then hook."""
+
+import os
+import sys
+
+import pytest
+from click.testing import CliRunner
+
+sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+
+import alakazam
+from alakazam import deploy_apps
+
+CONFIG = {"a.org": {
+    "nextcloud": {"app_domain": "cloud.a.org", "server": "a.org", "version": "2.0.0"},
+    "wekan": {"app_domain": "boards.a.org", "server": "a.org", "version": "2.0.0"},
+}}
+
+
+@pytest.fixture
+def steps(monkeypatch):
+    """Records what a deploy run does, in order, against an abra that answers every call."""
+    steps = []
+    monkeypatch.setattr(alakazam, "INSTANCE_CONFIGS", CONFIG)
+    monkeypatch.setattr(alakazam, "sleep", lambda seconds: steps.append(f"sleep:{seconds}"))
+    monkeypatch.setattr(alakazam, "execute_cmds",
+                        lambda config, **kw: steps.append(f"hook:{config['app_domain']}"))
+    monkeypatch.setattr(alakazam, "get_apps", lambda recipes, **kw: {
+        "a.org": [[app, config["app_domain"]] for app, config in CONFIG["a.org"].items()
+                  if not recipes or app in recipes]})
+    monkeypatch.setattr(alakazam, "get_apps_by_deployment", lambda recipes, deployed=True: {
+        "a.org": [[app, config["app_domain"]] + (["2.0.0"] if deployed else [])
+                  for app, config in CONFIG["a.org"].items() if not recipes or app in recipes]})
+
+    def abra(*args, **kwargs):
+        if args[:2] == ("app", "ps"):
+            steps.append(f"ps:{args[2]}")
+            return {"app": {"service": "app", "status": "healthy", "version": "2.0.0",
+                            "chaos": "false", "state": "running", "image": "img"}}
+        steps.append(" ".join(a for a in args if a))
+        return ""
+
+    monkeypatch.setattr(alakazam, "abra", abra)
+    return steps
+
+
+def selected(steps, prefix):
+    return [step for step in steps if step.startswith(prefix)]
+
+
+def run(args):
+    result = CliRunner().invoke(alakazam.deploy, ["-n"] + args, standalone_mode=False)
+    assert result.exit_code == 0, result.exception
+    return result
+
+
+class TestDeployHooks:
+    def test_the_hooks_do_not_switch_on_converge_checks(self, steps):
+        """Waiting for each app in turn is what the separate flag is for."""
+        run(["-e"])
+        assert all("--no-converge-checks" in s for s in selected(steps, "app deploy"))
+
+    def test_converge_checks_come_from_their_own_flag_only(self, steps):
+        run(["-c"])
+        assert not any("--no-converge-checks" in s for s in selected(steps, "app deploy"))
+
+    def test_every_app_is_deployed_before_the_first_hook(self, steps):
+        run(["-e"])
+        deploys = selected(steps, "app deploy")
+        assert max(steps.index(s) for s in deploys) < steps.index("hook:cloud.a.org")
+
+    def test_the_hooks_run_after_the_wait(self, steps):
+        """A hook would otherwise reach a container the deployment has not brought up yet."""
+        run(["-e"])
+        assert steps.index("ps:cloud.a.org") < steps.index("hook:cloud.a.org")
+        assert steps.index("ps:boards.a.org") < steps.index("hook:boards.a.org")
+
+    def test_the_health_is_only_read_after_the_services_settled(self, steps):
+        """A replaced service still reports its old container, which has been healthy all along."""
+        run(["-e"])
+        assert steps.index(f"sleep:{alakazam.PS_WAIT_SETTLE}") < steps.index("ps:cloud.a.org")
+        assert max(steps.index(s) for s in selected(steps, "app deploy")) \
+            < steps.index(f"sleep:{alakazam.PS_WAIT_SETTLE}")
+
+    def test_without_the_flag_nothing_is_hooked(self, steps):
+        run([])
+        assert selected(steps, "hook:") == []
+
+    def test_the_wait_happens_without_the_flag_too(self, steps):
+        """It is what tells a deploy run whether the apps came up at all."""
+        run([])
+        assert selected(steps, "ps:") == ["ps:cloud.a.org", "ps:boards.a.org"]
+
+    def test_a_filtered_run_waits_for_its_own_apps_only(self, steps):
+        run(["-e", "-r", "wekan"])
+        assert selected(steps, "ps:") == ["ps:boards.a.org"]
+        assert selected(steps, "hook:") == ["hook:boards.a.org"]
+
+
+class TestDeployAppsWithoutWaiting:
+    """setup arranges its own waiting, per dependency level and once for the whole environment."""
+
+    def test_the_wait_can_be_opted_out_of(self, steps):
+        deploy_apps({"a.org": [["nextcloud", "cloud.a.org"]]}, wait=False)
+        assert selected(steps, "ps:") == []
+        assert selected(steps, "sleep:") == []
+
+    def test_the_hooks_still_run_without_a_wait(self, steps):
+        deploy_apps({"a.org": [["nextcloud", "cloud.a.org"]]}, execute_hooks=True, wait=False)
+        assert selected(steps, "hook:") == ["hook:cloud.a.org"]
+
+    def test_nothing_to_deploy_skips_the_wait(self, steps):
+        """Waiting on a run that did not happen would report on unrelated apps."""
+        deploy_apps({})
+        assert steps == []
+
+
+class TestSettleOption:
+    def test_the_delay_can_be_set(self, steps):
+        run(["--settle", "5"])
+        assert "sleep:5" in steps
+        assert f"sleep:{alakazam.PS_WAIT_SETTLE}" not in steps
+
+    def test_zero_skips_the_delay_without_skipping_the_wait(self, steps):
+        """A converge-checked run has waited already, the health still has to be confirmed."""
+        run(["--settle", "0"])
+        assert selected(steps, "sleep:") == []
+        assert selected(steps, "ps:") == ["ps:cloud.a.org", "ps:boards.a.org"]
+
+    def test_the_default_is_used_when_it_is_not_given(self, steps):
+        run([])
+        assert f"sleep:{alakazam.PS_WAIT_SETTLE}" in steps

tests/test_env_updates.py

@@ -0,0 +1,144 @@
+"""Tests for how update_configs() applies a config's env/comment/uncomment keys to a .env file."""
+
+import logging
+import os
+import sys
+
+from dotenv import dotenv_values
+
+sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+
+from alakazam import format_env_value, set_extending_key, 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
+
+
+class TestQuotedEnvValues:
+    """
+    A value keeps the quotes of its YAML scalar, and jinja substitutes globals that carry quotes of
+    their own. Quotes of the same style then end the line early where it is read back, so which
+    style a configuration happens to use must not change the value that arrives in the .env file.
+    """
+
+    def test_both_quoting_styles_produce_the_same_line(self, tmp_path):
+        env = tmp_path / "app.env"
+        env.write_text("")
+        update_configs(env, {"env": {
+            "DOUBLE": '"["turn:a.org:443?transport=tcp"]"',
+            "SINGLE": '\'["turn:a.org:443?transport=tcp"]\'',
+        }})
+        assert env.read_text().splitlines() == [
+            'DOUBLE=\'["turn:a.org:443?transport=tcp"]\'',
+            'SINGLE=\'["turn:a.org:443?transport=tcp"]\'',
+        ]
+
+    def test_an_inner_quote_does_not_end_the_line(self, tmp_path):
+        """Otherwise the parser stops after "[" and reads the rest of the line as further keys."""
+        env = tmp_path / "app.env"
+        env.write_text("")
+        update_configs(env, {"env": {"TURN_URIS": '"["turn:a.org:443"]"'}})
+        assert dotenv_values(env) == {"TURN_URIS": '["turn:a.org:443"]'}
+
+    def test_the_other_style_is_preferred_over_escaping(self):
+        """It needs no escape at all, so it survives a reader that resolves none."""
+        assert format_env_value('"["a"]"') == '\'["a"]\''
+        assert format_env_value("'it's here'") == '"it\'s here"'
+
+    def test_a_value_that_expands_keeps_its_style_and_escapes(self):
+        """Single quotes would stop abra from expanding the "$", which changes what the app gets."""
+        assert format_env_value('"a "b" $VAR"') == '"a \\"b\\" $VAR"'
+
+    def test_an_already_escaped_quote_is_not_escaped_again(self):
+        assert format_env_value(r'"a \" b"') == r'"a \" b"'
+
+    def test_a_value_holding_both_styles_is_reported(self, caplog):
+        """Neither style can hold it, single quotes have no escape in abra's parser."""
+        with caplog.at_level(logging.WARNING):
+            format_env_value('\'it\'s "quoted"\'')
+        assert "single quote" in caplog.text
+
+    def test_a_value_without_quotes_is_written_unchanged(self):
+        assert format_env_value("plain value") == "plain value"
+
+    def test_a_non_string_passes_through(self):
+        assert format_env_value(8080) == 8080
+
+    def test_a_quoted_extending_value_is_not_wrapped_twice(self, tmp_path):
+        """Two layers of quotes read back as an empty string, which drops every compose file."""
+        env = tmp_path / "app.env"
+        env.write_text("")
+        set_extending_key(env, "COMPOSE_FILE", '"$COMPOSE_FILE:compose.mariadb.yml"')
+        assert dotenv_values(env)["COMPOSE_FILE"] == "$COMPOSE_FILE:compose.mariadb.yml"
+
+    def test_an_unquoted_extending_value_is_still_quoted_for_expansion(self, tmp_path):
+        """The quotes have to be double ones, abra expands the "$" only inside those."""
+        env = tmp_path / "app.env"
+        env.write_text("")
+        set_extending_key(env, "COMPOSE_FILE", "$COMPOSE_FILE:compose.mariadb.yml")
+        assert env.read_text() == 'COMPOSE_FILE="$COMPOSE_FILE:compose.mariadb.yml"\n'

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_latest_version.py

@@ -0,0 +1,68 @@
+"""Tests for reading the newest recipe version out of what 'abra recipe versions -m' writes."""
+
+import json
+import os
+import sys
+
+import click
+import pytest
+
+sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+
+import alakazam
+from alakazam import get_latest_version, parse_machine_output
+
+# one document per recipe version, each listing the images of that version, as abra writes it
+GRIST_VERSIONS = (
+    '[{"name":"gristlabs/grist-oss","service":"app","tag":"1.7.19","version":"3.6.4+1.7.19"},'
+    '{"name":"redis","service":"redis","tag":"8.10-alpine","version":"3.6.4+1.7.19"}]\n'
+    '[{"name":"gristlabs/grist-oss","service":"app","tag":"1.7.18","version":"3.6.3+1.7.18"},'
+    '{"name":"redis","service":"redis","tag":"8.10-alpine","version":"3.6.3+1.7.18"}]\n'
+    '[{"name":"gristlabs/grist","service":"app","tag":"1.1.12","version":"1.0.0+1.1.12"},'
+    '{"name":"postgres","service":"db","tag":"13","version":"1.0.0+1.1.12"}]\n'
+)
+
+
+class TestParseMachineOutput:
+    def test_several_documents_are_read_as_a_list(self):
+        """A plain json.loads() stops after the first line with "Extra data"."""
+        assert len(parse_machine_output(GRIST_VERSIONS)) == 3
+
+    def test_a_single_document_stays_one_document(self):
+        assert parse_machine_output('{"a.org": {"apps": []}}') == {"a.org": {"apps": []}}
+
+    def test_a_single_list_stays_one_list(self):
+        assert parse_machine_output('[{"name": "db_password"}]') == [{"name": "db_password"}]
+
+    def test_blank_lines_are_skipped(self):
+        assert parse_machine_output('{"a": 1}\n\n{"b": 2}\n') == [{"a": 1}, {"b": 2}]
+
+    def test_output_that_is_no_json_at_all_still_raises(self):
+        """Swallowing it would turn a broken abra call into an empty but plausible answer."""
+        with pytest.raises(json.JSONDecodeError):
+            parse_machine_output("FATA something went wrong")
+
+
+class TestGetLatestVersion:
+    def install(self, monkeypatch, output):
+        monkeypatch.setattr(alakazam, "abra", lambda *a, **k: parse_machine_output(output))
+
+    def test_the_newest_version_is_taken(self, monkeypatch):
+        self.install(monkeypatch, GRIST_VERSIONS)
+        assert get_latest_version("grist") == "3.6.4+1.7.19"
+
+    def test_the_order_of_the_documents_does_not_matter(self, monkeypatch):
+        self.install(monkeypatch, "\n".join(reversed(GRIST_VERSIONS.strip().splitlines())))
+        assert get_latest_version("grist") == "3.6.4+1.7.19"
+
+    def test_a_single_flat_document_is_read_too(self, monkeypatch):
+        """Which is what abra writes for a recipe that has exactly one version."""
+        self.install(monkeypatch, '[{"name":"redis","tag":"8.10","version":"1.0.0+1.1.12"}]')
+        assert get_latest_version("grist") == "1.0.0+1.1.12"
+
+    def test_a_recipe_without_versions_is_reported(self, monkeypatch):
+        """Reaching for the newest of nothing would end in an IndexError with no hint in it."""
+        monkeypatch.setattr(alakazam, "abra", lambda *a, **k: {})
+        with pytest.raises(click.ClickException) as excinfo:
+            get_latest_version("grist")
+        assert "no versions for grist" in excinfo.value.message

tests/test_local_scripts.py

@@ -168,7 +168,8 @@ class TestInstanceDomainThreading:
         monkeypatch.setattr(alakazam, "abra", lambda *a, **k: "")
         calls = []
         monkeypatch.setattr(alakazam, "execute_cmds", lambda app_config, **k: calls.append(k))
-        deploy_apps({"a.org": [["authentik", "login.a.org"]]}, execute_hooks=True)
+        # the health check is not what this is about, and it would need a whole abra to answer
+        deploy_apps({"a.org": [["authentik", "login.a.org"]]}, execute_hooks=True, wait=False)
         [kwargs] = calls
         assert kwargs["deploy"] is True
         assert kwargs["instance_domain"] == "a.org"

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"

tests/test_secret_exchange.py

@@ -0,0 +1,85 @@
+"""Tests for reading a secret out of a running app, and for what happens when that fails."""
+
+import logging
+import os
+import subprocess
+import sys
+
+import pytest
+
+sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+
+import alakazam
+from alakazam import get_running_containers, get_secret, run_over_ssh
+
+
+class TestGetRunningContainers:
+    def test_the_containers_are_returned(self, monkeypatch):
+        monkeypatch.setattr(alakazam, "abra", lambda *a, **k: [{"service": "app"}])
+        assert get_running_containers("cloud.a.org") == [{"service": "app"}]
+
+    def test_an_app_that_is_not_deployed_has_none(self, monkeypatch):
+        def abra(*args, **kwargs):
+            raise RuntimeError("abra -o app ps -m cloud.a.org\n STDERR: FATA cloud.a.org is not deployed?")
+
+        monkeypatch.setattr(alakazam, "abra", abra)
+        assert get_running_containers("cloud.a.org") == []
+
+    def test_any_other_failure_is_raised(self, monkeypatch):
+        """Reading an unreachable server as "not running" would skip the exchange without a word."""
+        def abra(*args, **kwargs):
+            raise RuntimeError("abra -o app ps -m cloud.a.org\n STDERR: ssh: connect to host")
+
+        monkeypatch.setattr(alakazam, "abra", abra)
+        with pytest.raises(RuntimeError, match="connect to host"):
+            get_running_containers("cloud.a.org")
+
+
+class TestGetSecret:
+    def test_the_run_asks_for_no_terminal(self, monkeypatch):
+        """A ci run has no terminal to give, and one would rewrite the line endings of the value."""
+        calls = []
+        monkeypatch.setattr(alakazam, "abra", lambda *a, **k: calls.append(a) or "s3cret")
+        assert get_secret("cloud.a.org", "db_password", ["app"]) == "s3cret"
+        assert calls[0][:4] == ("app", "run", "-t", "cloud.a.org")
+
+    def test_the_next_container_is_tried(self, monkeypatch):
+        def abra(*args, **kwargs):
+            if args[4] == "db":
+                raise RuntimeError("FATA no such container")
+            return "s3cret"
+
+        monkeypatch.setattr(alakazam, "abra", abra)
+        assert get_secret("cloud.a.org", "db_password", ["db", "app"]) == "s3cret"
+
+
+class TestRunOverSsh:
+    def result(self, returncode=0, stdout="", stderr=""):
+        return subprocess.CompletedProcess(["ssh"], returncode, stdout, stderr)
+
+    def test_the_output_is_returned_unchanged(self, monkeypatch):
+        """A trailing newline can be part of a secret, so nothing is stripped."""
+        monkeypatch.setattr(subprocess, "run", lambda *a, **k: self.result(stdout="s3cret\n"))
+        assert run_over_ssh("a.org", "cat x", "the read") == "s3cret\n"
+
+    def test_a_failing_command_is_raised(self, monkeypatch):
+        """It exited 1 and wrote nothing, which read as an empty secret before."""
+        monkeypatch.setattr(subprocess, "run",
+                            lambda *a, **k: self.result(returncode=1, stderr="Permission denied"))
+        with pytest.raises(RuntimeError) as excinfo:
+            run_over_ssh("a.org", "cat x", "the read")
+        assert "exited with code 1" in str(excinfo.value)
+        assert "Permission denied" in str(excinfo.value)
+
+    def test_a_silent_failure_is_still_raised(self, monkeypatch):
+        monkeypatch.setattr(subprocess, "run", lambda *a, **k: self.result(returncode=255))
+        with pytest.raises(RuntimeError, match="no output"):
+            run_over_ssh("a.org", "cat x", "the read")
+
+    def test_stderr_on_a_successful_command_is_reported(self, monkeypatch, caplog):
+        """ssh writes its own warnings there, and they explain an answer that looks wrong."""
+        monkeypatch.setattr(subprocess, "run",
+                            lambda *a, **k: self.result(stdout="s3cret", stderr="Pseudo-terminal will not be allocated"))
+        with caplog.at_level(logging.WARNING):
+            assert run_over_ssh("a.org", "cat x", "the read") == "s3cret"
+        assert "Pseudo-terminal" in caplog.text

tests/test_upgrade_hooks.py

@@ -0,0 +1,249 @@
+"""Tests for what an upgrade run does: deploy everything, wait for all of it, then hook."""
+
+import logging
+import os
+import sys
+
+import click
+import pytest
+from click.testing import CliRunner
+
+sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+
+import alakazam
+
+CONFIG = {"a.org": {
+    "nextcloud": {"app_domain": "cloud.a.org", "server": "a.org", "version": "2.0.0",
+                  "upgrade-hooks": ["app occ upgrade"]},
+    "wekan": {"app_domain": "boards.a.org", "server": "a.org", "version": "2.0.0"},
+}}
+
+# kept before any fixture replaces it, for the tests that check what a dry run actually prints
+REAL_EXECUTE_CMDS = alakazam.execute_cmds
+
+
+@pytest.fixture
+def versions():
+    """The version each app is deployed at, a test changes it to steer what needs upgrading."""
+    return {"nextcloud": "1.0.0", "wekan": "1.0.0"}
+
+
+@pytest.fixture
+def steps(monkeypatch, versions):
+    """Records what an upgrade run does, in order, against an abra that answers every call."""
+    steps = []
+    monkeypatch.setattr(alakazam, "INSTANCE_CONFIGS", CONFIG)
+    monkeypatch.setattr(alakazam, "sleep", lambda seconds: steps.append(f"sleep:{seconds}"))
+    monkeypatch.setattr(alakazam, "get_missing_secrets", lambda domain: [])
+    # a dry run reaches the same function, so the two have to be told apart here
+    monkeypatch.setattr(alakazam, "execute_cmds", lambda config, dry_run=False, **kw: steps.append(
+        f"{'show' if dry_run else 'hook'}:{config['app_domain']}"))
+
+    # the recipe filter has to be honoured here, it is what the wait narrows itself down with
+    def apps_by_deployment(recipes, deployed=True):
+        return {"a.org": [[app, CONFIG["a.org"][app]["app_domain"], version]
+                          for app, version in versions.items() if not recipes or app in recipes]}
+
+    def abra(*args, **kwargs):
+        if args[:2] == ("app", "ps"):
+            steps.append(f"ps:{args[2]}")
+            return {"app": {"service": "app", "status": "healthy", "version": "2.0.0",
+                            "chaos": "false", "state": "running", "image": "img"}}
+        # the planning phase asks for the release notes with the same command plus -r
+        if "-r" not in args:
+            steps.append(" ".join(a for a in args if a))
+        return ""
+
+    monkeypatch.setattr(alakazam, "get_apps_by_deployment", apps_by_deployment)
+    monkeypatch.setattr(alakazam, "abra", abra)
+    return steps
+
+
+def selected(steps, prefix):
+    return [step for step in steps if step.startswith(prefix)]
+
+
+def run(args, **kwargs):
+    result = CliRunner().invoke(alakazam.upgrade, args, standalone_mode=False, **kwargs)
+    assert result.exit_code == 0, result.exception
+    return result
+
+
+class TestUpgradeHooks:
+    def test_the_hooks_do_not_switch_on_converge_checks(self, steps):
+        """Waiting for each app in turn is what the separate flag is for."""
+        run(["-n", "-e"])
+        assert all("--no-converge-checks" in s for s in selected(steps, "app upgrade"))
+
+    def test_converge_checks_come_from_their_own_flag_only(self, steps):
+        run(["-n", "-c"])
+        assert not any("--no-converge-checks" in s for s in selected(steps, "app upgrade"))
+
+    def test_every_app_is_deployed_before_the_first_hook(self, steps):
+        run(["-n", "-e"])
+        deploys = selected(steps, "app upgrade")
+        assert max(steps.index(s) for s in deploys) < steps.index("hook:cloud.a.org")
+
+    def test_the_hooks_run_after_the_wait(self, steps):
+        """A hook would otherwise reach a container the upgrade has not brought up again yet."""
+        run(["-n", "-e"])
+        assert steps.index("ps:cloud.a.org") < steps.index("hook:cloud.a.org")
+        assert steps.index("ps:boards.a.org") < steps.index("hook:boards.a.org")
+
+    def test_the_health_is_only_read_after_the_services_settled(self, steps):
+        """A replaced service still reports its old container, which has been healthy all along."""
+        run(["-n", "-e"])
+        settle = steps.index(f"sleep:{alakazam.PS_WAIT_SETTLE}")
+        assert max(steps.index(s) for s in selected(steps, "app upgrade")) < settle
+        assert settle < steps.index("ps:cloud.a.org")
+
+    def test_without_the_flag_nothing_is_hooked(self, steps):
+        run(["-n"])
+        assert selected(steps, "hook:") == []
+
+    def test_the_wait_happens_without_the_flag_too(self, steps):
+        """It is what tells an upgrade run whether the apps came back at all."""
+        run(["-n"])
+        assert selected(steps, "ps:") == ["ps:cloud.a.org", "ps:boards.a.org"]
+
+    def test_a_filtered_run_waits_for_its_own_apps_only(self, steps):
+        run(["-n", "-e", "-r", "wekan"])
+        assert selected(steps, "ps:") == ["ps:boards.a.org"]
+        assert selected(steps, "hook:") == ["hook:boards.a.org"]
+
+    def test_an_app_already_at_its_version_is_neither_waited_for_nor_hooked(self, steps, versions):
+        versions["nextcloud"] = "2.0.0"
+        run(["-n", "-e"])
+        assert selected(steps, "ps:") == ["ps:boards.a.org"]
+        assert selected(steps, "hook:") == ["hook:boards.a.org"]
+
+    def test_an_app_with_an_unreadable_version_is_neither_waited_for_nor_hooked(self, steps, versions):
+        versions["nextcloud"] = "whatever"
+        run(["-n", "-e"])
+        assert selected(steps, "ps:") == ["ps:boards.a.org"]
+        assert selected(steps, "hook:") == ["hook:boards.a.org"]
+
+    def test_nothing_to_upgrade_skips_the_wait_entirely(self, steps, versions):
+        """Waiting on a run that did not happen would report on unrelated apps."""
+        versions.update({"nextcloud": "2.0.0", "wekan": "2.0.0"})
+        run(["-n", "-e"])
+        assert steps == []
+
+
+class TestDryRun:
+    """A dry run prints what would happen. Touching the server at all makes the flag pointless."""
+
+    def executed(self, steps):
+        return [step for step in steps if not step.startswith("show:")]
+
+    def test_an_interactive_dry_run_upgrades_nothing(self, steps):
+        """It still shows the release notes, so it asks a question that must not start anything."""
+        run(["--dry-run", "-e"], input="\nYES\n")
+        assert self.executed(steps) == []
+
+    def test_a_non_interactive_dry_run_upgrades_nothing(self, steps):
+        run(["-n", "--dry-run", "-e"])
+        assert self.executed(steps) == []
+
+    def test_an_interactive_dry_run_asks_nothing_about_upgrading(self, steps):
+        assert "Do you really want to upgrade" not in run(["--dry-run"], input="\n").output
+
+    def test_the_version_jump_is_shown_and_labelled(self, steps):
+        output = run(["--dry-run"], input="\n").output
+        assert "from" in output and "to" in output
+        assert "1.0.0" in output and "2.0.0" in output
+
+    def test_the_hooks_are_shown_when_they_would_run(self, steps, monkeypatch):
+        monkeypatch.setattr(alakazam, "execute_cmds", REAL_EXECUTE_CMDS)
+        assert "Run 'app occ upgrade' in cloud.a.org" in run(["--dry-run", "-e"], input="\n").output
+
+    def test_the_hooks_stay_out_without_the_flag(self, steps, monkeypatch):
+        """They would not run either, so showing them would misreport the run."""
+        monkeypatch.setattr(alakazam, "execute_cmds", REAL_EXECUTE_CMDS)
+        assert "app occ upgrade" not in run(["--dry-run"], input="\n").output
+
+    def test_a_shown_hook_is_not_executed(self, steps):
+        run(["--dry-run", "-e"], input="\n")
+        assert selected(steps, "show:") == ["show:cloud.a.org", "show:boards.a.org"]
+        assert selected(steps, "hook:") == []
+
+
+class TestSettleOption:
+    def test_the_delay_can_be_set(self, steps):
+        run(["-n", "--settle", "5"])
+        assert "sleep:5" in steps
+        assert f"sleep:{alakazam.PS_WAIT_SETTLE}" not in steps
+
+    def test_zero_skips_the_delay_without_skipping_the_wait(self, steps):
+        """A converge-checked run has waited already, the health still has to be confirmed."""
+        run(["-n", "--settle", "0"])
+        assert selected(steps, "sleep:") == []
+        assert selected(steps, "ps:") == ["ps:cloud.a.org", "ps:boards.a.org"]
+
+
+class TestAFailingApp:
+    """A server that refuses one app must not take the apps behind it down with it."""
+
+    @pytest.fixture
+    def failing(self, monkeypatch, steps):
+        """Lets the upgrade of cloud.a.org fail the way abra reports a rejected deployment."""
+        original = alakazam.abra
+
+        def abra(*args, **kwargs):
+            # the planning phase asks for the release notes with the same command plus -r
+            if args[:2] == ("app", "upgrade") and "-r" not in args and "cloud.a.org" in args:
+                raise RuntimeError("abra -o app upgrade -n cloud.a.org 2.0.0 \n STDERR: FATA "
+                                   "failed to update cloud_a_org_db: context deadline exceeded")
+            return original(*args, **kwargs)
+
+        monkeypatch.setattr(alakazam, "abra", abra)
+        return steps
+
+    def invoke(self, args):
+        return CliRunner().invoke(alakazam.upgrade, ["-n"] + args, standalone_mode=False)
+
+    def test_the_other_apps_are_still_upgraded(self, failing):
+        self.invoke(["-e"])
+        assert "app upgrade -n --no-converge-checks boards.a.org 2.0.0" in failing
+
+    def test_the_run_ends_on_the_failure(self, failing):
+        result = self.invoke(["-e"])
+        assert isinstance(result.exception, click.ClickException)
+        assert "1 of 2 app(s) failed to upgrade: cloud.a.org" in result.exception.message
+
+    def test_the_failing_app_is_neither_waited_for_nor_hooked(self, failing):
+        """Its state is unknown, so a hook against it would run on whatever is left there."""
+        self.invoke(["-e"])
+        assert selected(failing, "ps:") == ["ps:boards.a.org"]
+        assert selected(failing, "hook:") == ["hook:boards.a.org"]
+
+    def test_the_reason_is_reported(self, failing, caplog):
+        with caplog.at_level(logging.ERROR):
+            self.invoke(["-e"])
+        assert "context deadline exceeded" in caplog.text
+
+    def test_every_app_failing_skips_the_wait(self, steps, monkeypatch):
+        def abra(*args, **kwargs):
+            if args[:2] == ("app", "upgrade") and "-r" not in args:
+                raise RuntimeError("FATA failed to update: context deadline exceeded")
+            return ""
+
+        monkeypatch.setattr(alakazam, "abra", abra)
+        result = self.invoke(["-e"])
+        assert isinstance(result.exception, click.ClickException)
+        assert "2 of 2 app(s) failed" in result.exception.message
+        assert selected(steps, "ps:") == []
+
+    def test_a_failing_wait_does_not_hide_the_failing_app(self, failing, monkeypatch):
+        """One failure covering up another is how a broken app slips through a green pipeline."""
+        monkeypatch.setattr(alakazam, "wait_for_healthy_apps", lambda *a, **k: (_ for _ in ()).throw(
+            click.ClickException("1 service(s) still not healthy after 900s")))
+        message = self.invoke(["-e"]).exception.message
+        assert "cloud.a.org" in message
+        assert "still not healthy" in message
+
+    def test_the_hooks_stay_out_when_the_wait_failed(self, failing, monkeypatch):
+        monkeypatch.setattr(alakazam, "wait_for_healthy_apps", lambda *a, **k: (_ for _ in ()).throw(
+            click.ClickException("1 service(s) still not healthy after 900s")))
+        self.invoke(["-e"])
+        assert selected(failing, "hook:") == []

tests/test_upgrade_secrets.py

@@ -52,6 +52,9 @@ class TestUpgradeFillsThemIn:
             if args[:3] == ("app", "secret", "ls"):
                 return [{"name": n, "created on server": str(n not in missing).lower()}
                         for n in ("db_password", "oidc_secret")]
+            if args[:2] == ("app", "ps"):
+                return {"app": {"service": "app", "status": "healthy", "version": "2.0.0",
+                                "chaos": "false", "state": "running", "image": "img"}}
             steps.append(" ".join(a for a in args if a))
             return ""
         monkeypatch.setattr(alakazam, "abra", abra)