feat(deploy): run the hooks after one wait for all apps

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
@@ -88,6 +88,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 +483,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]:
@@ -916,6 +926,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 +1005,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]:
@@ -1422,22 +1505,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 +1531,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 +1923,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 +1937,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 +1975,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 +2001,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 +2033,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,13 +2058,19 @@ 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":
         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
@@ -1966,8 +2087,14 @@ def upgrade(recipes: Tuple[str], execute_hooks: bool, dry_run: bool, redeploy: b
                 sleep(redeploy_wait)
             print(f'deploy {app_domain}')
             print(abra("app", *upgrade_cmd))
-            if execute_hooks:
-                logging.info(f'execute commands for {app_domain}')
+        if not upgrade_cmds:
+            return
+        # 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 upgrade_cmds}), timeout, settle=settle)
+        if execute_hooks:
+            for app, app_config, _, instance_domain in upgrade_cmds:
+                logging.info(f"execute commands for {app_config.get('app_domain')}")
                 execute_cmds(app_config, upgrade=True, instance_domain=instance_domain)
 
 
@@ -2089,23 +2216,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]:
@@ -2413,8 +2539,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 +2558,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 +2640,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 +2781,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_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_upgrade_hooks.py

@@ -0,0 +1,179 @@
+"""Tests for what an upgrade 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
+
+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"]

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)