feat(upgrade): report the run in three buckets

README.md

@@ -147,6 +147,10 @@ To update all instances:
 
 1. **Update Environment Files**: `alakazam example.com.yml config` to refresh .env files. It's a good practice to keep these files under version control (e.g., in a Git repository at `~/.abra/example.com`) and review changes with `git diff` before proceeding.
 2. **Upgrade Applications**: `alakazam example.com.yml upgrade` to update all applications.
+    - an app that abra refuses and an app that does not become healthy both end the run non-zero,
+      the apps beside them are still upgraded and still get their hooks
+    - a failing upgrade hook is reported but does not end the run, so that the hooks can be fixed
+      one at a time
 
 ### Rotate Secrets
 
@@ -262,6 +266,13 @@ For each app/recipe the following `<app_configurations>` can be used:
     - this is useful for env variables that are used multiple times like `COMPOSE_FILE`
 - **`env`**: Sets values for environment variables.
 - **`*-hooks`**: Specifies `abra.sh` commands or local scripts to run at specific stages.
+    - **`pre-upgrade-hooks`**: commands that should be run before each upgrade, while the old
+      containers are still running
+        - they are a precondition rather than an addition, so unlike every other kind they run
+          without `--execute-hooks`; `--skip-pre-upgrade-hooks` leaves them out
+        - a failing one costs that app its upgrade and nothing else, the run then ends non-zero
+        - the command comes from the recipe version the configuration asks for, so it has to exist
+          there and has to work against the app that is still running the old one
     - **`initial-hooks`**: commands for initialisation
     - **`deploy-hooks`**: commands that should be run after each deployment
     - **`upgrade-hooks`**: commands that should be run after each upgrade

alakazam.py

@@ -61,8 +61,14 @@ GROUP_PATH = None  # path this run was invoked with, either an instance file or
 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 = 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_RETRIES = 8 # number of attempts per abra command that failed for a reason worth retrying
 ABRA_RETRY_DELAY = 2 # seconds before the first retry, doubled for every further one
+# lower case markers in the abra output for an app whose replaced container has not gone away yet,
+# which leaves abra unable to tell which one a command is meant for
+AMBIGUOUS_CONTAINER_ERRORS = (
+    "ambiguous container list",
+    "expected 1 container but found",
+)
 # lower case markers in the abra output that indicate an unreachable server instead of a failed operation
 CONNECTION_ERRORS = (
     "error during connect",
@@ -79,6 +85,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 +96,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
@@ -703,6 +712,27 @@ def is_connection_error(output: str) -> bool:
     return any(error in output.lower() for error in CONNECTION_ERRORS)
 
 
+def get_retry_reason(output: str) -> Optional[str]:
+    """
+    Names the transient condition an abra output shows, or nothing when the failure is a real one.
+
+    Both conditions leave the server untouched: an unreachable server never ran the command, and an
+    ambiguous container list is decided before the command is handed over. Retrying is therefore
+    safe even for a command that would not survive being run twice.
+
+    Args:
+        output (str): The combined stdout and stderr of the abra command
+
+    Returns:
+        str: What went wrong, phrased for the retry message, or None if it is worth no retry
+    """
+    if is_connection_error(output):
+        return "failed to reach the server"
+    if any(error in output.lower() for error in AMBIGUOUS_CONTAINER_ERRORS):
+        return "found more than one container, the replaced one has not gone away yet"
+    return None
+
+
 def run_streamed(command: List[str]) -> subprocess.CompletedProcess:
     """
     Runs a command and echoes its output line by line while it is still running.
@@ -761,12 +791,13 @@ def abra(*args: str, machine_output: bool = False, ignore_error: bool = False, s
         process = run_streamed(command) if stream else subprocess.run(command, capture_output=True)
         if not process.returncode or attempt == ABRA_RETRIES:
             break
-        if not is_connection_error(process.stdout.decode() + process.stderr.decode()):
+        reason = get_retry_reason(process.stdout.decode() + process.stderr.decode())
+        if not reason:
             break
         # back off exponentially: a server that drops connections during the ssh handshake is
         # usually throttling, and retrying every couple of seconds keeps it throttling
         delay = ABRA_RETRY_DELAY * 2 ** (attempt - 1)
-        logging.warning(f"attempt {attempt}/{ABRA_RETRIES} of '{' '.join(command)}' failed to reach the server, retry in {delay}s")
+        logging.warning(f"attempt {attempt}/{ABRA_RETRIES} of '{' '.join(command)}' {reason}, retry in {delay}s")
         sleep(delay)
     if process.stderr and ignore_error and not quiet:
         logging.warning(process.stderr.decode())
@@ -781,11 +812,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.
@@ -925,6 +975,43 @@ 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
@@ -939,7 +1026,7 @@ def set_extending_key(path: Path, key: str, value: str) -> None:
         key (str): The env var key.
         value (str): The value the key should be set to.
     """
-    targetline = f'{key}="{value}"'
+    targetline = '{}={}'.format(key, format_env_value(value, default_quote='"'))
     with open(path, "r") as file:
         lines = file.readlines()
     for i, line in enumerate(lines):
@@ -980,7 +1067,7 @@ def update_configs(path: Path, config: Dict[str, Any]) -> None:
             if key in extending_keys:
                 set_extending_key(path, key, value)
             else:
-                dotenv.set_key(path, key, value, quote_mode="never")
+                dotenv.set_key(path, key, format_env_value(value), quote_mode="never")
 
 
 def get_missing_secrets(domain: str) -> List[str]:
@@ -1152,18 +1239,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:
@@ -1235,6 +1312,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.
@@ -1249,7 +1351,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
@@ -1259,6 +1363,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
@@ -1286,8 +1414,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}")
 
@@ -1299,8 +1426,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
@@ -1342,7 +1469,7 @@ def insert_secrets_from_conf(domain: str, app_config: Dict[str, Any]) -> None:
                 insert_secret(domain, secret_name, secret)
 
 
-def run_local_script(tokens: List[str], app_domain: str, server: str, instance_domain: str, dry_run: bool = False, strict: bool = False) -> None:
+def run_local_script(tokens: List[str], app_domain: str, server: str, instance_domain: str, dry_run: bool = False, strict: bool = False) -> Optional[str]:
     """
     Run a local script hook. Relative paths are resolved against ROOT_PATH for execution. Logs an error and returns early if the script is not executable.
 
@@ -1354,6 +1481,9 @@ def run_local_script(tokens: List[str], app_domain: str, server: str, instance_d
         dry_run (bool): If True, prints the command but does not execute it.
         strict (bool): Abort on a non-executable script or a non-zero exit code instead of only logging it.
 
+    Returns:
+        str: Why the script failed, or None when it ran through
+
     Raises:
         click.ClickException: If the script is not executable or exits non-zero and strict is set
     """
@@ -1367,16 +1497,18 @@ def run_local_script(tokens: List[str], app_domain: str, server: str, instance_d
         if strict:
             raise click.ClickException(f"script '{cmd_display}' failed for {app_domain}: {message}")
         logging.error(message)
-        return
+        return message
     if dry_run:
-        return
+        return None
     # the script may print a secret it just created, so its output is captured and dropped
     result = subprocess.run([str(script_path)] + args, env=env, capture_output=HIDE_SECRETS)
     if result.returncode != 0:
         message = f"Script '{cmd_display}' exited with code {result.returncode}"
         if strict:
             raise click.ClickException(f"script '{cmd_display}' failed for {app_domain}: {message}")
-        logging.warning(message)
+        logging.error(message)
+        return message
+    return None
 
 
 def run_secret_hooks(domain: str, app_config: Dict[str, Any], instance_domain: str = "", strict: bool = False) -> None:
@@ -1632,7 +1764,7 @@ def run_readiness_hooks(domain: str, app_config: Dict[str, Any]) -> None:
         run_readiness_hook(domain, hook)
 
 
-def execute_cmds(app_config: Dict[str, Any], commands: Tuple[str] = tuple(), initial: bool = False, deploy: bool = False, upgrade: bool = False, dry_run: bool = False, chaos: bool = False, strict: bool = False, instance_domain: str = "") -> None:
+def execute_cmds(app_config: Dict[str, Any], commands: Tuple[str] = tuple(), pre_upgrade: bool = False, initial: bool = False, deploy: bool = False, upgrade: bool = False, dry_run: bool = False, chaos: bool = False, strict: bool = False, instance_domain: str = "") -> List[str]:
     """
     Execute post-deployment commands for an application based on the provided configuration.
     This can include running scripts or commands inside the application's environment.
@@ -1641,6 +1773,7 @@ def execute_cmds(app_config: Dict[str, Any], commands: Tuple[str] = tuple(), ini
         app_config (dict): A dictionary containing the deployment configuration and commands for an application.
         commands (list): A list of commands that should be executed in the following format:
             '<container_name> <abra.sh command> <optional arguments>'
+        pre_upgrade (bool): execute pre-upgrade-hooks, which run while the old containers still do
         initial (bool): execute initial-hooks
         deploy (bool): execute deploy-hooks
         upgrade (bool): execute upgrade-hooks
@@ -1649,38 +1782,55 @@ def execute_cmds(app_config: Dict[str, Any], commands: Tuple[str] = tuple(), ini
         instance_domain (str): The instance domain, used to set ALAKAZAM_INSTANCE_DOMAIN for script hooks.
 
     Returns:
-        None
+        list: The commands that failed, empty when all of them ran through. Always empty with
+            strict, which ends on the first failure instead of collecting it.
 
     Raises:
         click.ClickException: If a command fails and strict is set
     """
     domain = app_config['app_domain']
     server = app_config['server']
-    all_cmds = []
+    # kept grouped by kind rather than flattened, so that a dry run can say which kind it shows
+    groups = []
+    if pre_upgrade and (pre_upgrade_hooks:= app_config.get('pre-upgrade-hooks')):
+        groups.append(('pre-upgrade-hooks', pre_upgrade_hooks))
     if initial and (initial_hooks:= app_config.get('initial-hooks')):
-        all_cmds = all_cmds + initial_hooks
+        groups.append(('initial-hooks', initial_hooks))
     if deploy and (deploy_hooks:= app_config.get('deploy-hooks')):
-        all_cmds = all_cmds + deploy_hooks
+        groups.append(('deploy-hooks', deploy_hooks))
     if upgrade and (upgrade_hooks:= app_config.get('upgrade-hooks')):
-        all_cmds = all_cmds + upgrade_hooks
+        groups.append(('upgrade-hooks', upgrade_hooks))
     if commands:
-        all_cmds = all_cmds + list(commands)
+        groups.append(('--command', list(commands)))
     chaos_flag = ""
     if chaos:
         chaos_flag = "-C"
-    for cmd in all_cmds:
-        tokens = cmd.split()
-        container = tokens[0]
-        if container == "script" and len(tokens) >= 2 and resolve_path(tokens[1]).exists():
-            run_local_script(tokens, domain, server, instance_domain, dry_run, strict)
-            continue
-        print(f"Run '{cmd}' in {domain}:{container}", flush=True)
+    failed = []
+    for kind, cmds in groups:
+        # a real run has the deployment output between the kinds to tell them apart, a dry run
+        # prints them back to back and needs saying which side of the deployment each one is on
         if dry_run:
-            continue
-        try:
-            abra(*build_app_cmd(domain, cmd, chaos_flag), ignore_error=not strict, stream=True)
-        except RuntimeError as e:
-            raise click.ClickException(f"command '{cmd}' failed for {domain}: {e}")
+            print(f"{kind} for {domain}:")
+        for cmd in cmds:
+            tokens = cmd.split()
+            container = tokens[0]
+            if container == "script" and len(tokens) >= 2 and resolve_path(tokens[1]).exists():
+                if reason := run_local_script(tokens, domain, server, instance_domain, dry_run, strict):
+                    failed.append(cmd)
+                continue
+            print(f"Run '{cmd}' in {domain}:{container}", flush=True)
+            if dry_run:
+                continue
+            try:
+                # the error is raised here even without strict, so that a failure can be reported
+                # rather than only showing up in the streamed output
+                abra(*build_app_cmd(domain, cmd, chaos_flag), stream=True)
+            except RuntimeError as e:
+                if strict:
+                    raise click.ClickException(f"command '{cmd}' failed for {domain}: {e}")
+                logging.error(f"command '{cmd}' failed for {domain}: {e}")
+                failed.append(cmd)
+    return failed
 
 
 @click.group(context_settings={"help_option_names": ['-h', '--help']})
@@ -1885,8 +2035,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.
     """
@@ -1897,19 +2049,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)
@@ -1921,32 +2087,105 @@ 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)
+
+
+def run_upgrade_hooks(upgraded: List[Tuple[str, Dict[str, Any], str]], unhealthy: List[str]) -> List[Tuple[str, str]]:
+    """
+    Runs the upgrade hooks of every app that came back healthy and collects the ones that failed.
+
+    An app that never became healthy is left out: its hook would reach a container that is not
+    there, and the failure would say nothing beyond what the health check already said. A failing
+    hook is collected rather than raised, so that the apps behind it still get theirs.
+
+    Args:
+        upgraded (list): Tuples of app name, app configuration and instance domain
+        unhealthy (list): App domains whose services did not become healthy
+
+    Returns:
+        list: Tuples of app domain and the hook command that failed
+    """
+    failures = []
+    for app, app_config, instance_domain in upgraded:
+        domain = app_config.get('app_domain')
+        if domain in unhealthy:
+            logging.warning(f"{domain} is not healthy, its upgrade hooks are left out")
+            continue
+        logging.info(f"execute commands for {domain}")
+        failures += [(domain, cmd) for cmd in
+                     execute_cmds(app_config, upgrade=True, instance_domain=instance_domain)]
+    return failures
+
+
+def report_upgrade_run(total: int, failed: List[str], unhealthy: List[str],
+                       hook_failures: List[Tuple[str, str]]) -> None:
+    """
+    Reports what an upgrade run could not finish, and ends it when something needs a person.
+
+    An app that was refused and an app that never became healthy both leave the environment in a
+    state nobody asked for, so they end the run non-zero. A failing hook is reported only: hooks
+    still fail for reasons that are not failures, such as mariadb-upgrade refusing an already
+    upgraded database, and a verdict on them has to wait until those are sorted out.
+
+    Args:
+        total (int): How many apps the run set out to upgrade
+        failed (list): App domains abra refused to upgrade
+        unhealthy (list): App domains whose services did not become healthy
+        hook_failures (list): Tuples of app domain and the hook command that failed
+
+    Raises:
+        click.ClickException: If any app failed to upgrade or never became healthy
+    """
+    if hook_failures:
+        listed = ", ".join(f"{domain} '{cmd}'" for domain, cmd in hook_failures)
+        logging.error(f"{len(hook_failures)} upgrade hook(s) failed, "
+                      f"which does not end this run: {listed}")
+    problems = []
+    if failed:
+        problems.append(f"{len(failed)} of {total} app(s) failed to upgrade: {', '.join(failed)}")
+    if unhealthy:
+        problems.append(f"{len(unhealthy)} of {total} app(s) did not become healthy: "
+                        f"{', '.join(unhealthy)}")
+    if problems:
+        raise click.ClickException("\n".join(problems))
 
 
 @cli.command()
 @click.option('recipes', '-r', '--recipe', multiple=True, metavar='<RecipeName>', help='Filter for selcted recipes, this option can be specified multiple times.')
 @click.option('-e', '--execute-hooks', is_flag=True, help='run post-upgrade commands.')
+@click.option('skip_pre_upgrade_hooks', '--skip-pre-upgrade-hooks', is_flag=True,
+              help='leave out the pre-upgrade-hooks, which run unasked otherwise.')
 @click.option('-d', '--dry-run', is_flag=True, help="don't execute the upgrade process")
 @click.option('-rd', '--redeploy', is_flag=True, help="use undeploy and deploy for the updating process")
 @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:
+def upgrade(recipes: Tuple[str], execute_hooks: bool, skip_pre_upgrade_hooks: bool, dry_run: bool,
+            redeploy: bool, 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.
+    A pre-upgrade hook runs before an app is replaced, while its old containers are still up. It is a precondition rather than an addition, so it needs no --execute-hooks and a failing one costs its app the upgrade.
     """
     deployed_instance_apps = get_apps_by_deployment(recipes, deployed=True)
     upgrade_cmds = []
@@ -1970,7 +2209,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)
@@ -1995,32 +2234,71 @@ 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:
+        # shown instead of run, so that a dry run answers what the hooks would have done. The
+        # pre-upgrade ones are not behind --execute-hooks and are therefore always listed
+        if not skip_pre_upgrade_hooks:
+            for _, app_config, _, instance_domain in upgrade_cmds:
+                execute_cmds(app_config, pre_upgrade=True, dry_run=True,
+                             instance_domain=instance_domain)
+        if execute_hooks:
+            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))
+            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 not skip_pre_upgrade_hooks:
+                    # before the undeploy below, which would leave the hook nothing to act on
+                    execute_cmds(app_config, pre_upgrade=True, strict=True,
+                                 instance_domain=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, click.ClickException) 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))
+        unhealthy, hook_failures = [], []
+        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
+                pending = get_unhealthy_apps(tuple({app for app, *_ in upgraded}), timeout,
+                                             settle=settle)
+                unhealthy = sorted({domain for domain, _ in pending})
+                if pending:
+                    logging.error(f"{len(pending)} service(s) still not healthy after {timeout}s:"
+                                  f"\n{format_pending_containers(pending)}")
+            except click.ClickException as error:
+                # with nothing to read the health off, no app can be told apart from another
+                logging.error(error.message)
+                unhealthy = sorted(config.get('app_domain') for _, config, _ in upgraded)
             if execute_hooks:
-                logging.info(f'execute commands for {app_domain}')
-                execute_cmds(app_config, upgrade=True, instance_domain=instance_domain)
+                hook_failures = run_upgrade_hooks(upgraded, unhealthy)
+        report_upgrade_run(len(upgrade_cmds), failed, unhealthy, hook_failures)
 
 
 @cli.command()
@@ -2056,12 +2334,13 @@ def undeploy_apps(instance_apps: Dict[str, List[List]]) -> None:
 @cli.command()
 @click.option('recipes', '-r', '--recipe', multiple=True, metavar='<RecipeName>', help='Filter for selcted recipes, this option can be specified multiple times.')
 @click.option('commands', '-c', '--command', multiple=True, metavar="'<container_name> <abra.sh command> <optional arguments>'", help='commands that should be executed')
+@click.option('pre_upgrade', '-p', '--pre-upgrade', is_flag=True, help='execute pre-upgrade-hooks from config')
 @click.option('-i', '--initial', is_flag=True, help='execute initial-hooks from config')
 @click.option('-d', '--deploy', is_flag=True, help='execute deploy-hooks from config')
 @click.option('-u', '--upgrade', is_flag=True, help='execute upgrade-hooks from config')
 @click.option('-l', '--list-cmds', is_flag=True, help="only show cmds, don't execute them")
 @click.option('-C', '--chaos', is_flag=True, help="execute in chaos mode")
-def cmd(recipes: Tuple[str], commands: Tuple[str], initial: bool, deploy: bool, upgrade: bool, list_cmds: bool = False, chaos: bool = False) -> None:
+def cmd(recipes: Tuple[str], commands: Tuple[str], pre_upgrade: bool, initial: bool, deploy: bool, upgrade: bool, list_cmds: bool = False, chaos: bool = False) -> None:
     """
     Execute commands for all specified applications based on the provided configuration.
     """
@@ -2078,7 +2357,7 @@ def cmd(recipes: Tuple[str], commands: Tuple[str], initial: bool, deploy: bool,
                 print(f"{domain} is not deployed")
                 continue
             logging.info(f'execute commands for {domain}')
-            execute_cmds(app_config, commands=commands, initial=initial, deploy=deploy, upgrade=upgrade, dry_run=list_cmds, chaos=chaos, instance_domain=instance_domain)
+            execute_cmds(app_config, commands=commands, pre_upgrade=pre_upgrade, initial=initial, deploy=deploy, upgrade=upgrade, dry_run=list_cmds, chaos=chaos, instance_domain=instance_domain)
 
 
 @cli.command()
@@ -2141,23 +2420,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]:
@@ -2338,8 +2616,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
 
@@ -2465,8 +2748,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.
 
@@ -2482,7 +2767,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()
@@ -2563,22 +2849,35 @@ 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 get_unhealthy_apps(recipes: Tuple[str], timeout: int, verbose: bool = False, settle: int = 0) -> List[Tuple[str, Dict[str, Any]]]:
     """
-    Blocks until no service of the selected apps is starting, unknown or unhealthy any more.
+    Waits for the services of the selected apps to settle and reports the ones that never did.
 
-    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 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: reading
     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
+        recipes (tuple): Recipes to read, 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
+
+    Returns:
+        list: Tuples of app domain and container for every service that is still not healthy,
+            empty when all of them are
 
     Raises:
-        click.ClickException: If nothing is deployed, or if services are still not healthy when the timeout is reached
+        click.ClickException: If nothing is deployed
     """
+    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)
@@ -2589,15 +2888,49 @@ def wait_for_healthy_apps(recipes: Tuple[str], timeout: int, verbose: bool = Fal
         if not pending:
             print_containers(containers, verbose)
             print("All services are healthy")
-            return
+            return []
         if time() >= deadline:
             break
         waiting = ", ".join(f"{domain} {container['service']} ({container['status']})" for domain, container in pending)
         print(f"{len(pending)} service(s) not healthy yet, check again in {PS_WAIT_INTERVAL}s: {waiting}", flush=True)
         sleep(PS_WAIT_INTERVAL)
     print_containers(containers, verbose)
-    report = "\n".join(f"  {domain} {container['service']}: {container['status']}" for domain, container in pending)
-    raise click.ClickException(f"{len(pending)} service(s) still not healthy after {timeout}s:\n{report}")
+    return pending
+
+
+def format_pending_containers(pending: List[Tuple[str, Dict[str, Any]]]) -> str:
+    """
+    Renders the services that never became healthy as one indented line each.
+
+    Args:
+        pending (list): Tuples of app domain and container information
+
+    Returns:
+        str: One line per service, naming its app and its status
+    """
+    return "\n".join(f"  {domain} {container['service']}: {container['status']}"
+                     for domain, container in pending)
+
+
+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.
+
+    For a caller that has something left to do for the apps that did come back, get_unhealthy_apps()
+    reports them instead of ending the run.
+
+    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 pending := get_unhealthy_apps(recipes, timeout, verbose, settle):
+        raise click.ClickException(f"{len(pending)} service(s) still not healthy after {timeout}s:"
+                                   f"\n{format_pending_containers(pending)}")
 
 
 def get_deployment_traces(recipes: Tuple[str]) -> List[str]:
@@ -2695,7 +3028,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)

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

@@ -1,11 +1,14 @@
 """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 update_configs
+from alakazam import format_env_value, set_extending_key, update_configs
 
 
 class TestUnknownConfigKeys:
@@ -74,3 +77,68 @@ class TestExtendingEnvKeys:
         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_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,5 +1,6 @@
 """Tests for which abra failures may be retried."""
 
+import logging
 import os
 import sys
 
@@ -9,7 +10,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
+from alakazam import abra, get_retry_reason, is_connection_error
 
 
 class FakeProcess:
@@ -21,19 +22,22 @@ class FakeProcess:
 
 UNREACHABLE = b"FATA error during connect: kex_exchange_identification: Connection closed by remote host"
 REJECTED = b"FATA login.a.org is still deployed"
+AMBIGUOUS = (b"WARN ambiguous container list received, prompting for input\n"
+             b"? which container are you looking for?\nFATA EOF")
+
+
+@pytest.fixture
+def runs(monkeypatch):
+    """Hands out a prepared result per abra call and records what was slept between them."""
+    calls = {"results": [], "slept": []}
+    monkeypatch.setattr(alakazam, "sleep", calls["slept"].append)
+    monkeypatch.setattr(alakazam.subprocess, "run", lambda cmd, capture_output: calls["results"].pop(0))
+    return calls
 
 
 class TestRetryLoop:
     """A server that drops the ssh handshake is usually throttling, so the retries have to spread out."""
 
-    @pytest.fixture
-    def runs(self, monkeypatch):
-        calls = {"results": [], "slept": []}
-        monkeypatch.setattr(alakazam, "sleep", calls["slept"].append)
-        monkeypatch.setattr(alakazam.subprocess, "run",
-                            lambda cmd, capture_output: calls["results"].pop(0))
-        return calls
-
     def test_a_successful_command_does_not_retry(self, runs):
         runs["results"] = [FakeProcess(0)]
         abra("app", "ls")
@@ -83,3 +87,35 @@ class TestConnectionErrors:
     ])
     def test_operational_failures_are_not_retried(self, output):
         assert not is_connection_error(output)
+
+
+class TestAmbiguousContainer:
+    """
+    An app that was just updated keeps its replaced container for a while, and abra then cannot
+    tell which of the two a command is meant for. It asks, and a run without a terminal ends on EOF.
+    """
+
+    @pytest.mark.parametrize("output", [
+        "WARN ambiguous container list received, prompting for input\nFATA EOF",
+        "FATA expected 1 container but found 2: files_app.1.pxo3 (created About a minute ago)",
+    ])
+    def test_it_is_worth_another_attempt(self, output):
+        assert get_retry_reason(output) == ("found more than one container, "
+                                            "the replaced one has not gone away yet")
+
+    def test_an_unreachable_server_keeps_its_own_reason(self):
+        assert get_retry_reason("kex_exchange_identification") == "failed to reach the server"
+
+    def test_a_rejected_operation_is_worth_none(self):
+        assert get_retry_reason("FATA login.a.org is still deployed") is None
+
+    def test_the_command_is_repeated_until_one_container_is_left(self, runs):
+        runs["results"] = [FakeProcess(1, AMBIGUOUS), FakeProcess(1, AMBIGUOUS), FakeProcess(0)]
+        abra("app", "cmd", "-T", "files.a.org", "app", "--", "run_occ", "upgrade")
+        assert runs["slept"] == [alakazam.ABRA_RETRY_DELAY, alakazam.ABRA_RETRY_DELAY * 2]
+
+    def test_the_reason_is_reported(self, runs, caplog):
+        runs["results"] = [FakeProcess(1, AMBIGUOUS), FakeProcess(0)]
+        with caplog.at_level(logging.WARNING):
+            abra("app", "cmd", "-T", "files.a.org", "app", "--", "run_occ", "upgrade")
+        assert "the replaced one has not gone away yet" in caplog.text

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,467 @@
+"""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
+
+# only nextcloud carries a pre-upgrade hook, so that "not configured" is distinguishable from
+# "configured but not run"
+CONFIG = {"a.org": {
+    "nextcloud": {"app_domain": "cloud.a.org", "server": "a.org", "version": "2.0.0",
+                  "pre-upgrade-hooks": ["db update_mongo_version"],
+                  "upgrade-hooks": ["app occ upgrade"]},
+    "wekan": {"app_domain": "boards.a.org", "server": "a.org", "version": "2.0.0",
+              "upgrade-hooks": ["app ready"]},
+}}
+
+# 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: [])
+    def execute_cmds(config, dry_run=False, **kw):
+        """
+        Labels a hook call by kind, and keeps quiet for an app that configures none of that kind.
+
+        selected() matches on a prefix, so the pre-upgrade label needs one of its own rather than a
+        suffix: "hook:pre:..." would be picked up by every assertion on "hook:".
+        """
+        kind, key = ("pre-hook", "pre-upgrade-hooks") if kw.get("pre_upgrade") else ("hook", "upgrade-hooks")
+        if config.get(key):
+            steps.append(f"{'show:' if dry_run else ''}{kind}:{config['app_domain']}")
+        return []   # the commands that failed, which execute_cmds reports back
+
+    monkeypatch.setattr(alakazam, "execute_cmds", execute_cmds)
+
+    # 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 unhealthy(*domains):
+    """What get_unhealthy_apps() reports for services that never settled."""
+    return [(domain, {"service": "app", "status": "starting"}) for domain in domains]
+
+
+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:pre-hook:cloud.a.org",
+                                            "show:hook:cloud.a.org", "show:hook:boards.a.org"]
+        assert selected(steps, "hook:") == []
+        assert selected(steps, "pre-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_an_unhealthy_app_does_not_hide_the_failing_one(self, failing, monkeypatch):
+        """One failure covering up another is how a broken app slips through a green pipeline."""
+        monkeypatch.setattr(alakazam, "get_unhealthy_apps", lambda *a, **k: unhealthy("boards.a.org"))
+        message = self.invoke(["-e"]).exception.message
+        assert "failed to upgrade: cloud.a.org" in message
+        assert "did not become healthy: boards.a.org" in message
+
+
+class TestPreUpgradeHooks:
+    """
+    A pre-upgrade hook prepares an app while its old containers still run, which is the only moment
+    some migrations have: mongo refuses to start on data whose feature compatibility version is two
+    majors behind the binary, so the version has to be raised before the binary is replaced.
+    """
+
+    def test_it_runs_before_that_app_is_deployed(self, steps):
+        run(["-n"])
+        upgrade = next(s for s in steps if "app upgrade" in s and "cloud.a.org" in s)
+        assert steps.index("pre-hook:cloud.a.org") < steps.index(upgrade)
+
+    def test_it_runs_before_the_undeploy_of_a_redeploy(self, steps):
+        """After an undeploy there is nothing left for the hook to read or change."""
+        run(["-n", "--redeploy"])
+        undeploy = next(s for s in steps if "app undeploy" in s and "cloud.a.org" in s)
+        assert steps.index("pre-hook:cloud.a.org") < steps.index(undeploy)
+
+    def test_it_runs_without_execute_hooks(self, steps):
+        """Unlike the other kinds: skipping a precondition does not skip work, it breaks the app."""
+        run(["-n"])
+        assert selected(steps, "pre-hook:") == ["pre-hook:cloud.a.org"]
+        assert selected(steps, "hook:") == []
+
+    def test_an_app_that_configures_none_gets_none(self, steps):
+        run(["-n"])
+        assert "pre-hook:boards.a.org" not in steps
+
+    def test_an_app_already_at_its_version_gets_none(self, steps, versions):
+        versions["nextcloud"] = "2.0.0"
+        run(["-n"])
+        assert selected(steps, "pre-hook:") == []
+
+    def test_the_flag_leaves_them_out(self, steps):
+        run(["-n", "--skip-pre-upgrade-hooks"])
+        assert selected(steps, "pre-hook:") == []
+        assert any("app upgrade" in s for s in steps)
+
+    def test_a_dry_run_shows_them_and_runs_nothing(self, steps):
+        run(["--dry-run"], input="\n")
+        assert selected(steps, "show:") == ["show:pre-hook:cloud.a.org"]
+        assert selected(steps, "pre-hook:") == []
+        assert not any("app upgrade" in s and "-r" not in s for s in steps)
+
+    def test_a_dry_run_honours_the_skip(self, steps):
+        run(["--dry-run", "--skip-pre-upgrade-hooks"], input="\n")
+        assert selected(steps, "show:") == []
+
+
+class TestAFailingPreUpgradeHook:
+    """It is a precondition, so it costs its own app the upgrade and no other app anything."""
+
+    @pytest.fixture
+    def failing(self, monkeypatch, steps):
+        def execute_cmds(config, dry_run=False, **kw):
+            if kw.get("pre_upgrade"):
+                if not config.get("pre-upgrade-hooks"):
+                    return
+                # what execute_cmds(strict=True) raises for a command the recipe does not have
+                raise click.ClickException(
+                    "command 'db update_mongo_version' failed for cloud.a.org: "
+                    "FATA no db function in abra.sh")
+            steps.append(f"hook:{config['app_domain']}")
+            return []
+
+        monkeypatch.setattr(alakazam, "execute_cmds", execute_cmds)
+        return steps
+
+    def invoke(self, args):
+        return CliRunner().invoke(alakazam.upgrade, ["-n"] + args, standalone_mode=False)
+
+    def test_that_app_is_not_upgraded(self, failing):
+        self.invoke([])
+        assert not any("app upgrade" in s and "cloud.a.org" in s for s in failing)
+
+    def test_the_other_app_still_is(self, failing):
+        self.invoke([])
+        assert any("app upgrade" in s and "boards.a.org" in s for s in failing)
+
+    def test_the_run_ends_naming_the_app(self, failing):
+        result = self.invoke([])
+        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):
+        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([])
+        assert "no db function in abra.sh" in caplog.text
+
+
+class TestDryRunNamesTheHookKind:
+    """
+    A real run has "deploy <domain>" between the kinds; a dry run prints them back to back, so the
+    kind has to be named or a pre-upgrade hook is indistinguishable from an identical later one.
+    """
+
+    @pytest.fixture
+    def output(self, steps, monkeypatch):
+        monkeypatch.setattr(alakazam, "execute_cmds", REAL_EXECUTE_CMDS)
+        return run(["--dry-run", "-e"], input="\n").output
+
+    def test_both_kinds_are_named(self, output):
+        assert "pre-upgrade-hooks for cloud.a.org:" in output
+        assert "upgrade-hooks for cloud.a.org:" in output
+
+    def test_the_pre_upgrade_kind_comes_first(self, output):
+        assert output.index("pre-upgrade-hooks for cloud.a.org:") < output.index("upgrade-hooks for")
+
+    def test_an_app_without_the_kind_does_not_announce_it(self, output):
+        assert "pre-upgrade-hooks for boards.a.org:" not in output
+
+    def test_a_real_run_names_no_kind(self, steps, monkeypatch):
+        """There the deployment output separates them, and the label would only be noise."""
+        monkeypatch.setattr(alakazam, "execute_cmds", REAL_EXECUTE_CMDS)
+        output = run(["-n", "-e"]).output
+        assert "upgrade-hooks for" not in output
+        assert "Run 'db update_mongo_version' in cloud.a.org:db" in output
+
+
+class TestAnUnhealthyApp:
+    """
+    An app that does not come back must not cost the apps that did their hooks: a run over twenty
+    apps would otherwise leave nineteen of them half migrated because of one.
+    """
+
+    @pytest.fixture
+    def sick(self, monkeypatch, steps):
+        monkeypatch.setattr(alakazam, "get_unhealthy_apps", lambda *a, **k: unhealthy("cloud.a.org"))
+        return steps
+
+    def invoke(self, args):
+        return CliRunner().invoke(alakazam.upgrade, ["-n"] + args, standalone_mode=False)
+
+    def test_the_healthy_app_still_gets_its_hooks(self, sick):
+        self.invoke(["-e"])
+        assert selected(sick, "hook:") == ["hook:boards.a.org"]
+
+    def test_the_unhealthy_app_does_not(self, sick):
+        """Its hook would reach a container that is not there."""
+        self.invoke(["-e"])
+        assert "hook:cloud.a.org" not in sick
+
+    def test_it_is_reported_and_ends_the_run(self, sick):
+        result = self.invoke(["-e"])
+        assert isinstance(result.exception, click.ClickException)
+        assert "1 of 2 app(s) did not become healthy: cloud.a.org" in result.exception.message
+
+    def test_a_health_check_that_reads_nothing_distrusts_every_app(self, steps, monkeypatch):
+        """Without a status for any of them, none of them can be told apart from a broken one."""
+        monkeypatch.setattr(alakazam, "get_unhealthy_apps", lambda *a, **k: (_ for _ in ()).throw(
+            click.ClickException("no apps are deployed, there is nothing to wait for")))
+        result = self.invoke(["-e"])
+        assert selected(steps, "hook:") == []
+        assert "boards.a.org" in result.exception.message
+        assert "cloud.a.org" in result.exception.message
+
+
+class TestAFailingUpgradeHook:
+    """
+    Reported but not a verdict: hooks still fail for reasons that are not failures, so they are
+    collected until those are sorted out and only then become something the run ends on.
+    """
+
+    @pytest.fixture
+    def broken(self, monkeypatch, steps):
+        def execute_cmds(config, dry_run=False, **kw):
+            if kw.get("pre_upgrade"):
+                return []
+            steps.append(f"hook:{config['app_domain']}")
+            return ["app occ upgrade"] if config["app_domain"] == "cloud.a.org" else []
+
+        monkeypatch.setattr(alakazam, "execute_cmds", execute_cmds)
+        return steps
+
+    def invoke(self, args):
+        return CliRunner().invoke(alakazam.upgrade, ["-n"] + args, standalone_mode=False)
+
+    def test_the_run_still_succeeds(self, broken):
+        result = self.invoke(["-e"])
+        assert result.exit_code == 0, result.exception
+
+    def test_it_is_reported(self, broken, caplog):
+        with caplog.at_level(logging.ERROR):
+            self.invoke(["-e"])
+        assert "1 upgrade hook(s) failed" in caplog.text
+        assert "cloud.a.org 'app occ upgrade'" in caplog.text
+
+    def test_the_other_apps_keep_their_hooks(self, broken):
+        self.invoke(["-e"])
+        assert selected(broken, "hook:") == ["hook:cloud.a.org", "hook:boards.a.org"]
+
+    def test_it_is_reported_next_to_a_failure_that_does_end_the_run(self, broken, monkeypatch, caplog):
+        monkeypatch.setattr(alakazam, "get_unhealthy_apps", lambda *a, **k: unhealthy("boards.a.org"))
+        with caplog.at_level(logging.ERROR):
+            result = self.invoke(["-e"])
+        assert "upgrade hook(s) failed" in caplog.text
+        assert "did not become healthy: boards.a.org" in result.exception.message

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)