Refactor run_command error handling

github_scripts/get_git_sources.py

@@ -1,8 +1,9 @@
-# *****************************COPYRIGHT*******************************
+# -----------------------------------------------------------------------------
 # (C) Crown copyright Met Office. All rights reserved.
-# For further details please refer to the file COPYRIGHT.txt
-# which you should have received as part of this distribution.
-# *****************************COPYRIGHT*******************************
+# The file LICENCE, distributed with this code, contains details of the terms
+# under which the code may be used.
+# -----------------------------------------------------------------------------
+
 """
 Clone sources for a rose-stem run for use with git bdiff module in scripts
 """
@@ -13,10 +14,16 @@
 from pathlib import Path
 from shutil import rmtree
 import shlex
+import logging
+
+logger = logging.getLogger(__name__)
 
 
 def run_command(
-    command: str, rval: bool = False, check: bool = True
+    command: str,
+    check: bool = True,
+    capture: bool = True,
+    timeout: int = 300
 ) -> Optional[subprocess.CompletedProcess]:
     """
     Run a subprocess command and return the result object
@@ -25,23 +32,32 @@ def run_command(
     Outputs:
         - result object from subprocess.run
     """
-    command = shlex.split(command)
-    result = subprocess.run(
-        command,
-        capture_output=True,
-        text=True,
-        timeout=300,
-        shell=False,
-        check=False,
-    )
-    if check and result.returncode:
-        print(result.stdout, end="\n\n\n")
-        raise RuntimeError(
-            f"[FAIL] Issue found running command {command}\n\n{result.stderr}"
+
+    args = shlex.split(command)
+
+    try:
+        # Note: text=True and capture_output=True have high overhead
+        # for large buffers. Use capture=False for fire-and-forget tasks.
+        result = subprocess.run(
+            args,
+            capture_output=capture,
+            text=capture,
+            timeout=timeout,
+            shell=False,
+            check=False
         )
-    if rval:
+        if check and result.returncode != 0:
+            err_msg = (result.stderr or "").strip()
+            logger.error(f"[FAIL] Command failed: {command}\nError: {err_msg}")
+            raise subprocess.CalledProcessError(
+                result.returncode, args, output=result.stdout, stderr=result.stderr
+            )
         return result
 
+    except (subprocess.TimeoutExpired, FileNotFoundError) as e:
+        logger.error(f"[FAIL] Execution error for '{args[0]}': {e}")
+        raise
+
 
 def clone_repo_mirror(
     source: str, repo_ref: str, parent: str, mirror_loc: Path, loc: Path

github_scripts/rose_stem_extract_source.py

@@ -1,24 +1,24 @@
 #!/usr/bin/env python3
-# *****************************COPYRIGHT*******************************
+# -----------------------------------------------------------------------------
 # (C) Crown copyright Met Office. All rights reserved.
-# For further details please refer to the file COPYRIGHT.txt
-# which you should have received as part of this distribution.
-# *****************************COPYRIGHT*******************************
+# The file LICENCE, distributed with this code, contains details of the terms
+# under which the code may be used.
+# -----------------------------------------------------------------------------
+
 """
 Clone sources for a rose-stem run for use with git bdiff module in scripts
-Only intended for use with rose-stem suites that have provided appropriate environment
-variables
+Only intended for use with rose-stem suites that have provided appropriate
+environment variables
 """
 
 import os
-from datetime import datetime
 from pathlib import Path
 from ast import literal_eval
 from get_git_sources import clone_repo, clone_repo_mirror, sync_repo
-from typing import Dict
+from datetime import datetime
 
 
-def set_https(dependencies: Dict) -> Dict:
+def set_https(dependencies: dict) -> dict:
     """
     Change sources in a dependencies dictions to use https instead of ssh
     """
@@ -35,32 +35,48 @@ def set_https(dependencies: Dict) -> Dict:
 
 
 def main() -> None:
+    """
+    1. Read environment variables for:
+        SOURCE_DIRECTORY - location to clone sources,
+        DEPENDENCIES - dictionary of dependencies,
+        USE_TOKENS - whether to use tokens for https URLs,
+        USE_MIRRORS - whether to use local git mirrors,
+        GIT_MIRROR_LOC - location of local git mirrors
+    2. For each dependency in DEPENDENCIES, clone or sync the source
+    3. If USE_TOKENS is True, modify the source URLs to use https
+    4. If USE_MIRRORS is True, clone from local mirrors at GIT_MIRROR_LOC
+    """
 
     clone_loc = Path(os.environ["SOURCE_DIRECTORY"])
-
-    dependencies: Dict = literal_eval(os.environ["DEPENDENCIES"])
+    dependencies: dict = literal_eval(os.environ["DEPENDENCIES"])
 
     if os.environ.get("USE_TOKENS", "False") == "True":
         dependencies = set_https(dependencies)
 
     for dependency, values in dependencies.items():
-
-        print(
-            f"Extracting {dependency} at time {datetime.now()} "
-            f"using source {values['source']} and ref {values['ref']}"
-        )
-
         loc = clone_loc / dependency
 
         if ".git" in values["source"]:
             if os.environ.get("USE_MIRRORS", "False") == "True":
                 mirror_loc = Path(os.environ["GIT_MIRROR_LOC"]) / values["parent"]
+                print(
+                    f"[{datetime.now().strftime('%Y-%m-%d %H:%M:%S')}] Cloning "
+                    f"{dependency} from {mirror_loc} at ref {values['ref']}"
+                )
                 clone_repo_mirror(
                     values["source"], values["ref"], values["parent"], mirror_loc, loc
                 )
             else:
+                print(
+                    f"[{datetime.now().strftime('%Y-%m-%d %H:%M:%S')}] Cloning "
+                    f"{dependency} from {values['source']} at ref {values['ref']}"
+                )
                 clone_repo(values["source"], values["ref"], loc)
         else:
+            print(
+                f"[{datetime.now().strftime('%Y-%m-%d %H:%M:%S')}] Syncing "
+                f"{dependency} at ref {values['ref']}"
+            )
             sync_repo(values["source"], values["ref"], loc)