Address review findings in mirror_images.py

- Fix silent except-pass when parsing existing lock file (now warns)
- Add error handling for future.result() in cmd_mirror check phase
- Remove dead code in resolve_digest (overwritten cmd variable)
- Move hashlib/fnmatch/glob imports to top level
- Add docker as copy tool via buildx imagetools create
- Log failures in list_tags instead of silently returning empty list
- Wrap json.loads in try-except for skopeo tag listing
- Validate YAML structure in parse_mirror_yaml and parse_lock_yaml
- Validate input in cmd_add for empty/malformed YAML
- Replace subprocess find with glob.glob in lint scanners
- Add _read_lines helper with error handling for file reads in lint
- Simplify redundant default expression in parse_mirror_yaml

Author: pawelchcki

bin/mirror_images.py

@@ -11,6 +11,9 @@
 """
 
 import argparse
+import fnmatch
+import glob
+import hashlib
 import json
 import os
 import re
@@ -53,10 +56,11 @@ def _find_digest_tool() -> str:
 
 def _find_copy_tool() -> str:
     """Find a tool that can copy images between registries."""
-    tool = _find_tool(["crane", "skopeo"])
+    tool = _find_tool(["crane", "skopeo", "docker"])
     if not tool:
-        print("No image copy tool found. Install one of: crane, skopeo",
-              file=sys.stderr)
+        print(
+            "No image copy tool found. Install one of: crane, skopeo, docker",
+            file=sys.stderr)
         sys.exit(1)
     return tool
 
@@ -71,10 +75,6 @@ def resolve_digest(image_ref: str, tool: str) -> str:
             f"docker://{image_ref}"
         ]
     elif tool in ("docker", "podman", "nerdctl"):
-        cmd = [tool, "buildx", "imagetools", "inspect", "--format",
-               "{{.Manifest.Digest}}", image_ref] if tool == "docker" else \
-              [tool, "image", "inspect", "--format", "{{.Digest}}", image_ref]
-        # Simpler: use manifest inspect and parse JSON
         cmd = [tool, "manifest", "inspect", image_ref]
     else:
         raise ValueError(f"Unknown tool: {tool}")
@@ -87,9 +87,9 @@ def resolve_digest(image_ref: str, tool: str) -> str:
     if tool in ("crane", "skopeo"):
         return result.stdout.strip()
 
-    # docker/podman/nerdctl: parse manifest JSON for the digest header
-    # The digest is in the descriptor, but we can compute it from the raw manifest
-    import hashlib
+    # docker/podman/nerdctl: compute digest from raw manifest output.
+    # Note: this may differ from the registry's canonical digest if the
+    # tool reformats the JSON. Prefer crane or skopeo for accurate digests.
     raw = result.stdout.strip().encode()
     return f"sha256:{hashlib.sha256(raw).hexdigest()}"
 
@@ -131,6 +131,10 @@ def copy_image(source_ref: str, target: str, tool: str) -> tuple[bool, str]:
             tool, "copy", "--all", f"docker://{source_ref}",
             f"docker://{target}"
         ]
+    elif tool == "docker":
+        cmd = [
+            tool, "buildx", "imagetools", "create", "-t", target, source_ref
+        ]
     else:
         raise ValueError(f"Unknown copy tool: {tool}")
 
@@ -142,25 +146,39 @@ def copy_image(source_ref: str, target: str, tool: str) -> tuple[bool, str]:
 
 def list_tags(image_repo: str, tool: str) -> list[str]:
     """List all tags for a repository."""
+    if tool in ("docker", "podman", "nerdctl"):
+        return []
+
     if tool == "crane":
         result = subprocess.run(["crane", "ls", image_repo],
                                 capture_output=True,
                                 text=True)
-        if result.returncode == 0:
-            return result.stdout.strip().splitlines()
     elif tool == "skopeo":
         result = subprocess.run(
             ["skopeo", "list-tags", f"docker://{image_repo}"],
             capture_output=True,
             text=True,
         )
-        if result.returncode == 0:
-            data = json.loads(result.stdout)
-            return data.get("Tags", [])
-    elif tool in ("docker", "podman", "nerdctl"):
-        # No good way to list tags with docker CLI; skip tag resolution
+    else:
+        return []
+
+    if result.returncode != 0:
+        print(
+            f"  [warning] Failed to list tags for {image_repo}: "
+            f"{result.stderr.strip()}",
+            file=sys.stderr)
+        return []
+
+    if tool == "crane":
+        return result.stdout.strip().splitlines()
+
+    try:
+        data = json.loads(result.stdout)
+        return data.get("Tags", [])
+    except json.JSONDecodeError as exc:
+        print(f"  [warning] Failed to parse tags for {image_repo}: {exc}",
+              file=sys.stderr)
         return []
-    return []
 
 
 # ---------------------------------------------------------------------------
@@ -193,14 +211,17 @@ def parse_mirror_yaml(path: str) -> dict[str, str]:
     with open(path) as f:
         entries = yaml.safe_load(f)
 
+    if not isinstance(entries, list):
+        raise ValueError(f"Expected a YAML list in {path}")
+
     images = {}
     for entry in entries:
         if isinstance(entry, str):
             images[entry] = f"{DEST_REGISTRY}/{entry}"
         elif isinstance(entry, dict):
             for source, opts in entry.items():
-                target = opts.get("target", f"{DEST_REGISTRY}/{source}"
-                                  ) if opts else f"{DEST_REGISTRY}/{source}"
+                opts = opts or {}
+                target = opts.get("target", f"{DEST_REGISTRY}/{source}")
                 images[source] = target
     return images
 
@@ -209,6 +230,10 @@ def parse_lock_yaml(path: str) -> dict[str, dict]:
     """Parse mirror_images.lock.yaml. Returns {source: {digest, target, tag}}."""
     with open(path) as f:
         data = yaml.safe_load(f)
+    if not isinstance(data, dict) or "images" not in data:
+        raise ValueError(
+            f"Malformed lock file {path}: expected a YAML mapping with an 'images' key"
+        )
     return data["images"]
 
 
@@ -262,7 +287,9 @@ def _check(tag: str):
         try:
             d = resolve_digest(f"{repo}:{tag}", tool)
             return tag, d
-        except Exception:
+        except Exception as exc:
+            print(f"    [debug] Could not resolve {repo}:{tag}: {exc}",
+                  file=sys.stderr)
             return tag, None
 
     futures = {pool.submit(_check, tag): tag for tag in candidates}
@@ -317,8 +344,11 @@ def cmd_lock(args: argparse.Namespace) -> int:
             existing = parse_lock_yaml(output_path)
             results.update(existing)
             print(f"Loaded {len(results)} existing entries from {output_path}")
-        except Exception:
-            pass
+        except Exception as exc:
+            print(
+                f"WARNING: Could not parse existing lock file {output_path}: {exc}",
+                file=sys.stderr)
+            print("Resolving all images from scratch.", file=sys.stderr)
 
     # Only resolve images not already in the lock file
     to_resolve = {
@@ -427,7 +457,13 @@ def _check(source: str, info: dict):
             for src, info in images.items()
         }
         for future in as_completed(futures):
-            source, info, present = future.result()
+            src = futures[future]
+            try:
+                source, info, present = future.result()
+            except Exception as exc:
+                print(f"  ERROR checking {src}: {exc}", file=sys.stderr)
+                to_copy.append((src, images[src]))
+                continue
             if present:
                 already_present += 1
                 print(f"  =  {source} (already mirrored)")
@@ -492,56 +528,46 @@ def _is_external(img: str) -> bool:
             return False
         return True
 
+    def _read_lines(filepath):
+        try:
+            with open(filepath) as f:
+                return list(enumerate(f, 1))
+        except OSError as exc:
+            print(f"  [warning] Could not read {filepath}: {exc}",
+                  file=sys.stderr)
+            return []
+
     def _scan_dockerfiles():
-        result = subprocess.run(
-            [
-                "find", PROJECT_DIR, "-name", "Dockerfile*", "-not", "-path",
-                "*/.git/*"
-            ],
-            capture_output=True,
-            text=True,
-        )
-        for filepath in result.stdout.splitlines():
-            if not filepath.strip():
+        for filepath in glob.glob(os.path.join(PROJECT_DIR, "**/Dockerfile*"),
+                                  recursive=True):
+            if "/.git/" in filepath:
                 continue
-            with open(filepath) as f:
-                for lineno, line in enumerate(f, 1):
-                    stripped = line.strip()
-                    m = re.match(r"^FROM\s+(\S+)", stripped, re.IGNORECASE)
-                    if m and _is_external(m.group(1)):
-                        hits.append((filepath, lineno, stripped, m.group(1)))
-                    for m in re.finditer(r"--from=(\S+)", stripped):
-                        img = m.group(1)
-                        if ("/" in img or ":" in img) and _is_external(img):
-                            hits.append((filepath, lineno, stripped, img))
+            for lineno, line in _read_lines(filepath):
+                stripped = line.strip()
+                m = re.match(r"^FROM\s+(\S+)", stripped, re.IGNORECASE)
+                if m and _is_external(m.group(1)):
+                    hits.append((filepath, lineno, stripped, m.group(1)))
+                for m in re.finditer(r"--from=(\S+)", stripped):
+                    img = m.group(1)
+                    if ("/" in img or ":" in img) and _is_external(img):
+                        hits.append((filepath, lineno, stripped, img))
 
     def _scan_yaml_image_fields():
-        result = subprocess.run(
-            [
-                "find", PROJECT_DIR, "(", "-name", "*.yml", "-o", "-name",
-                "*.yaml", ")", "-not", "-path", "*/.git/*", "-not", "-path",
-                "*/.gitlab/*"
-            ],
-            capture_output=True,
-            text=True,
-        )
-        for filepath in result.stdout.splitlines():
-            if not filepath.strip():
-                continue
-            with open(filepath) as f:
-                for lineno, line in enumerate(f, 1):
+        for pattern in ("**/*.yml", "**/*.yaml"):
+            for filepath in glob.glob(os.path.join(PROJECT_DIR, pattern),
+                                      recursive=True):
+                if "/.git/" in filepath or "/.gitlab/" in filepath:
+                    continue
+                for lineno, line in _read_lines(filepath):
                     m = re.match(r"^\s+image:\s+['\"]?(\S+?)['\"]?\s*$", line)
                     if m and _is_external(m.group(1)):
                         hits.append(
                             (filepath, lineno, line.rstrip(), m.group(1)))
 
     def _scan_ci_matrices():
-        import glob
         for filepath in glob.glob(
                 os.path.join(PROJECT_DIR, ".gitlab", "build-and-test-*.yml")):
-            with open(filepath) as f:
-                lines = f.readlines()
-            for lineno, line in enumerate(lines, 1):
+            for lineno, line in _read_lines(filepath):
                 m = re.search(r'BASE_IMAGE:\s*\[(.+)\]', line)
                 if m:
                     for img_m in re.finditer(r'"([^"]+)"', m.group(1)):
@@ -610,6 +636,11 @@ def cmd_add(args: argparse.Namespace) -> int:
     with open(args.mirror_yaml) as f:
         entries = yaml.safe_load(f)
 
+    if not isinstance(entries, list):
+        print(f"ERROR: {args.mirror_yaml} is empty or not a YAML list.",
+              file=sys.stderr)
+        return 1
+
     existing = set()
     for entry in entries:
         if isinstance(entry, str):
@@ -671,8 +702,6 @@ def cmd_relock(args: argparse.Namespace) -> int:
     command to re-resolve them. Patterns use fnmatch-style wildcards
     (e.g. 'nginx:1.29.*', 'openresty/*').
     """
-    import fnmatch
-
     lock_path = args.output or LOCK_YAML
     if not os.path.exists(lock_path):
         print(f"Lock file not found: {lock_path}", file=sys.stderr)