From 456db0a3e261bc2a71fb76b72fdefdb9ce642621 Mon Sep 17 00:00:00 2001 From: Simone Date: Sun, 17 May 2026 16:30:34 +0200 Subject: [PATCH] refactor(artifacts): harmonize Windows to Linux raw-file collection Windows produced an intermediate C:\CI\output\artifacts.zip that upload-artifact then re-zipped (zip-in-zip), while Linux deposited raw files. Make both paths consistent: the build stages raw outputs into a collect dir; collect zips it only as a transport archive, fetches, and extracts into the host artifact dir; actions/upload-artifact produces the single final archive. - _windows_build: param artifact_zip -> output_dir; stage raw files (in-place when source IS the collect dir, else wipe+copy), no Compress-Archive; absolute artifact-source resolved. - _windows_collect: zip guest dir as transport, fetch, extract (mirror _linux_collect); guest_path is now the collect directory. - job.py / build_run CLI: pass output_dir; rename --guest-artifact-zip -> --guest-output-dir. - Tests updated for the raw-file contract. Co-Authored-By: Claude Opus 4.7 --- src/ci_orchestrator/commands/artifacts.py | 46 ++++++++++++---- src/ci_orchestrator/commands/build.py | 65 +++++++++++++---------- src/ci_orchestrator/commands/job.py | 4 +- tests/python/test_commands_artifacts.py | 65 +++++++++++++++++------ tests/python/test_commands_build.py | 5 +- tests/python/test_commands_job.py | 4 +- 6 files changed, 131 insertions(+), 58 deletions(-) diff --git a/src/ci_orchestrator/commands/artifacts.py b/src/ci_orchestrator/commands/artifacts.py index f6b669e..e972a1b 100644 --- a/src/ci_orchestrator/commands/artifacts.py +++ b/src/ci_orchestrator/commands/artifacts.py @@ -16,6 +16,7 @@ import json import shlex import tarfile import tempfile +import zipfile from datetime import UTC, datetime from pathlib import Path @@ -133,19 +134,44 @@ def _windows_collect( include_logs: bool, ) -> None: cred = KeyringCredentialStore().get(credential_target) + remote_zip = "C:\\CI\\ci-artifacts-transfer.zip" with WinRmTransport(ip_address, cred.username, cred.password) as t: - # Verify artifact exists. + # guest_path is the collect directory (raw build outputs). + # Verify it exists and is non-empty. probe = t.run( - f"if (Test-Path {_ps_quote(guest_path)}) {{ 'OK' }} else {{ 'MISSING' }}", + f"if ((Test-Path {_ps_quote(guest_path)}) -and " + f"(Get-ChildItem -Force -- {_ps_quote(guest_path)})) " + f"{{ 'OK' }} else {{ 'MISSING' }}", check=False, ) if "MISSING" in probe.stdout: - raise click.ClickException(f"artifact not found in guest: {guest_path}") + raise click.ClickException( + f"artifact not found or empty in guest: {guest_path}" + ) - artifact_name = Path(guest_path).name - host_dest = host_dir / artifact_name - click.echo(f"[artifacts collect] fetching {artifact_name}") - t.fetch(guest_path, str(host_dest)) + # Zip the directory contents as a single transport file, fetch, + # extract into host_dir. Mirrors the Linux tar flow so both OSes + # deposit raw files; the final single archive is produced by + # actions/upload-artifact (no zip-in-zip). + t.run( + f"if (Test-Path {_ps_quote(remote_zip)}) " + f"{{ Remove-Item {_ps_quote(remote_zip)} -Force }}; " + f"Compress-Archive -Path (Join-Path {_ps_quote(guest_path)} '*') " + f"-DestinationPath {_ps_quote(remote_zip)} -Force" + ) + click.echo(f"[artifacts collect] fetching artifacts from {guest_path}") + try: + with tempfile.TemporaryDirectory() as td: + local_zip = Path(td) / "artifacts.zip" + t.fetch(remote_zip, str(local_zip)) + with zipfile.ZipFile(local_zip) as zf: + zf.extractall(host_dir) + finally: + t.run( + f"Remove-Item {_ps_quote(remote_zip)} -Force " + f"-ErrorAction SilentlyContinue", + check=False, + ) if include_logs: # List log files in the build dir and fetch each one. @@ -169,9 +195,9 @@ def _windows_collect( err=True, ) - if not host_dest.is_file() or host_dest.stat().st_size == 0: + if not any(host_dir.iterdir()): raise click.ClickException( - f"artifact missing or empty after fetch: {host_dest}" + f"no artifacts after collect from guest: {guest_path}" ) @@ -190,7 +216,7 @@ def _windows_collect( "--guest-artifact-path", "guest_artifact_path", required=True, - help="Guest path: file (Windows) or directory (Linux).", + help="Guest collect directory (Windows and Linux).", ) @click.option( "--host-artifact-dir", diff --git a/src/ci_orchestrator/commands/build.py b/src/ci_orchestrator/commands/build.py index 26f6aca..2ac3e6a 100644 --- a/src/ci_orchestrator/commands/build.py +++ b/src/ci_orchestrator/commands/build.py @@ -231,7 +231,7 @@ def _windows_build( ip_address: str, credential_target: str, workdir: str, - artifact_zip: str, + output_dir: str, host_source_dir: str | None, clone_url: str | None, clone_branch: str, @@ -251,7 +251,7 @@ def _windows_build( # Prepare workdir + artifact output directory. prep_ps = ( f"$work = {_ps_quote(workdir)}; " - f"$out = Split-Path {_ps_quote(artifact_zip)} -Parent; " + f"$out = {_ps_quote(output_dir)}; " "if (-not (Test-Path $out)) { New-Item -ItemType Directory -Path $out -Force | Out-Null }; " "if (Test-Path $work) { Remove-Item $work -Recurse -Force }; " "New-Item -ItemType Directory -Path $work -Force | Out-Null" @@ -346,34 +346,44 @@ def _windows_build( click.echo("[build run] artifact packaging skipped (--skip-artifact).") return - # Package the artifact source into the requested zip on the guest. + # Stage the artifact source into the collect dir as raw files + # (no intermediate zip — `actions/upload-artifact` produces the + # single final archive; mirrors the Linux flow, avoids zip-in-zip). # artifact_source may be absolute (e.g. C:\CI\output) or relative - # to the build workdir. PowerShell Join-Path does NOT treat a - # rooted second segment as absolute (it concatenates, yielding - # C:\CI\build\C:\CI\output), so resolve it explicitly. - if build_command: - src_rel = artifact_source or "dist" - else: - src_rel = "bin\\CIOutput" + # to the build workdir; PowerShell Join-Path does not treat a + # rooted segment as absolute, so resolve explicitly. When the + # source IS the collect dir (build wrote there directly), collect + # in place — do NOT wipe it (that deleted the build output). + src_rel = (artifact_source or "dist") if build_command else "bin\\CIOutput" pkg_ps = ( f"$work = {_ps_quote(workdir)}; " f"$rel = {_ps_quote(src_rel)}; " "$src = if ([System.IO.Path]::IsPathRooted($rel)) " "{ $rel } else { Join-Path $work $rel }; " - f"$zip = {_ps_quote(artifact_zip)}; " - "if (-not (Test-Path $src)) { " - " throw \"[build run] artifact source not found: $src\" }; " - "$dir = Split-Path $zip -Parent; " - "if (-not (Test-Path $dir)) { New-Item -ItemType Directory -Path $dir -Force | Out-Null }; " - "if (Test-Path $zip) { Remove-Item $zip -Force }; " - "if (Test-Path $src -PathType Leaf) { " - " Compress-Archive -Path $src -DestinationPath $zip -Force " + f"$out = {_ps_quote(output_dir)}; " + "$sf = [System.IO.Path]::GetFullPath($src).TrimEnd('\\'); " + "$of = [System.IO.Path]::GetFullPath($out).TrimEnd('\\'); " + "$inPlace = ($sf -ieq $of) -or " + "$sf.StartsWith($of + '\\', " + "[System.StringComparison]::OrdinalIgnoreCase); " + "if ($inPlace) { " + " if (-not (Test-Path $out) -or " + " -not (Get-ChildItem -Force -- $out)) { " + " throw \"[build run] artifact source not found or empty: $out\" } " "} else { " - " Compress-Archive -Path (Join-Path $src '*') -DestinationPath $zip -Force }" + " if (-not (Test-Path $src)) { " + " throw \"[build run] artifact source not found: $src\" }; " + " if (Test-Path $out) { Remove-Item $out -Recurse -Force }; " + " New-Item -ItemType Directory -Path $out -Force | Out-Null; " + " if (Test-Path $src -PathType Leaf) { " + " Copy-Item $src -Destination $out -Force " + " } else { " + " Copy-Item (Join-Path $src '*') -Destination $out -Recurse -Force } " + "}" ) t.run(pkg_ps) - click.echo(f"[build run] artifact written to {artifact_zip}") + click.echo(f"[build run] artifact staged at {output_dir}") # ---------------------------------------------------------------- CLI @@ -426,10 +436,11 @@ def _windows_build( help="Workdir inside the guest (defaults: Win=C:\\CI\\build, Linux=/opt/ci/build).", ) @click.option( - "--guest-artifact-zip", - "guest_artifact_zip", + "--guest-output-dir", + "guest_output_dir", default=None, - help="Artifact zip path inside the Windows guest (default: C:\\CI\\output\\artifacts.zip).", + help="Collect dir inside the Windows guest (default: C:\\CI\\output). " + "Raw build outputs are staged here; upload-artifact zips them.", ) @click.option( "--guest-linux-work-dir", @@ -468,7 +479,7 @@ def build_run( gitea_credential_target: str, build_command: str, guest_work_dir: str | None, - guest_artifact_zip: str | None, + guest_output_dir: str | None, guest_linux_work_dir: str | None, guest_linux_output_dir: str, guest_artifact_source: str, @@ -485,7 +496,7 @@ def build_run( * ``--host-source-dir`` — local directory zipped/tarred and uploaded; * ``--clone-url`` — ``git clone`` invoked inside the guest. - Phase C hook: ``--guest-work-dir`` and ``--guest-artifact-zip`` are + Phase C hook: ``--guest-work-dir`` and ``--guest-output-dir`` are parameterised; defaults are applied here, never inside the transport layer. """ @@ -533,14 +544,14 @@ def build_run( raise click.ClickException(str(exc)) from exc else: workdir = guest_work_dir or "C:\\CI\\build" - artifact_zip = guest_artifact_zip or "C:\\CI\\output\\artifacts.zip" + output_dir = guest_output_dir or "C:\\CI\\output" target = credential_target or config.guest_cred_target try: _windows_build( ip_address=ip_address, credential_target=target, workdir=workdir, - artifact_zip=artifact_zip, + output_dir=output_dir, host_source_dir=host_source_dir, clone_url=clone_url, clone_branch=clone_branch, diff --git a/src/ci_orchestrator/commands/job.py b/src/ci_orchestrator/commands/job.py index 9d4c824..5635727 100644 --- a/src/ci_orchestrator/commands/job.py +++ b/src/ci_orchestrator/commands/job.py @@ -492,7 +492,7 @@ def job( ip_address=ip_address, credential_target=target, workdir="C:\\CI\\build", - artifact_zip="C:\\CI\\output\\artifacts.zip", + output_dir="C:\\CI\\output", host_source_dir=None, clone_url=authed_repo_url, clone_branch=branch, @@ -526,7 +526,7 @@ def job( _windows_collect( ip_address=ip_address, credential_target=target, - guest_path="C:\\CI\\output\\artifacts.zip", + guest_path="C:\\CI\\output", host_dir=job_artifact_dir, include_logs=True, ) diff --git a/tests/python/test_commands_artifacts.py b/tests/python/test_commands_artifacts.py index 3d981c3..bdef687 100644 --- a/tests/python/test_commands_artifacts.py +++ b/tests/python/test_commands_artifacts.py @@ -4,6 +4,7 @@ from __future__ import annotations import json import tarfile +import zipfile from pathlib import Path from typing import Any, ClassVar @@ -78,8 +79,34 @@ def _patch(monkeypatch: pytest.MonkeyPatch) -> None: # ── Windows happy path ──────────────────────────────────────────────────── +def _zip_payload(tmp_path: Path) -> bytes: + """Build a real zip whose contents the fake transport will 'fetch'.""" + src = tmp_path / "zsrc" + src.mkdir() + (src / "binary.bin").write_bytes(b"\x01\x02\x03") + (src / "log.txt").write_text("hello", encoding="utf-8") + zip_local = tmp_path / "transfer.zip" + with zipfile.ZipFile(zip_local, "w") as zf: + for f in src.iterdir(): + zf.write(f, arcname=f.name) + return zip_local.read_bytes() + + +def _win_transport_with_zip(payload: bytes) -> type: + class _WinTransport(_FakeTransport): + def fetch(self, remote_path: str, local_path: str) -> None: + self.fetched.append((remote_path, str(local_path))) + Path(local_path).write_bytes(payload) + + return _WinTransport + + def test_collect_windows_happy_path(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: _patch(monkeypatch) + payload = _zip_payload(tmp_path) + monkeypatch.setattr( + artifacts_module, "WinRmTransport", _win_transport_with_zip(payload) + ) out_dir = tmp_path / "artifacts" result = CliRunner().invoke( cli, @@ -89,18 +116,19 @@ def test_collect_windows_happy_path(monkeypatch: pytest.MonkeyPatch, tmp_path: P "--ip-address", "10.0.0.7", "--guest-artifact-path", - "C:\\CI\\output\\artifacts.zip", + "C:\\CI\\output", "--host-artifact-dir", str(out_dir), ], ) assert result.exit_code == 0, result.output - assert (out_dir / "artifacts.zip").is_file() - instance = _FakeTransport.instances[0] - assert any("Test-Path" in c for c in instance.runs) - assert instance.fetched == [ - ("C:\\CI\\output\\artifacts.zip", str(out_dir / "artifacts.zip")) - ] + # Raw files extracted into host_dir (no zip-in-zip). + assert (out_dir / "binary.bin").read_bytes() == b"\x01\x02\x03" + assert (out_dir / "log.txt").read_text(encoding="utf-8") == "hello" + cmds = _FakeTransport.instances[0].runs + assert any("Compress-Archive" in c for c in cmds) + # Temp transport zip on the guest is cleaned up. + assert any("Remove-Item" in c and "ci-artifacts-transfer.zip" in c for c in cmds) def test_collect_windows_artifact_missing(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: @@ -109,7 +137,9 @@ def test_collect_windows_artifact_missing(monkeypatch: pytest.MonkeyPatch, tmp_p class _Missing(_FakeTransport): def __init__(self, *a: Any, **kw: Any) -> None: super().__init__(*a, **kw) - self.run_overrides["Test-Path"] = _FakeResult(stdout="MISSING\r\n") + self.run_overrides["-and (Get-ChildItem"] = _FakeResult( + stdout="MISSING\r\n" + ) monkeypatch.setattr(artifacts_module, "WinRmTransport", _Missing) result = CliRunner().invoke( @@ -120,22 +150,23 @@ def test_collect_windows_artifact_missing(monkeypatch: pytest.MonkeyPatch, tmp_p "--ip-address", "10.0.0.7", "--guest-artifact-path", - "C:\\CI\\output\\artifacts.zip", + "C:\\CI\\output", "--host-artifact-dir", str(tmp_path / "out"), ], ) assert result.exit_code != 0 - assert "artifact not found" in result.output + assert "artifact not found or empty" in result.output def test_collect_windows_include_logs(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: _patch(monkeypatch) + payload = _zip_payload(tmp_path) - class _WithLogs(_FakeTransport): + class _WithLogs(_win_transport_with_zip(payload)): # type: ignore[misc] def __init__(self, *a: Any, **kw: Any) -> None: super().__init__(*a, **kw) - self.run_overrides["Get-ChildItem"] = _FakeResult( + self.run_overrides["C:\\CI\\build"] = _FakeResult( stdout="C:\\CI\\build\\one.log\r\nC:\\CI\\build\\sub\\two.log\r\n" ) @@ -148,7 +179,7 @@ def test_collect_windows_include_logs(monkeypatch: pytest.MonkeyPatch, tmp_path: "--ip-address", "10.0.0.7", "--guest-artifact-path", - "C:\\CI\\output\\artifacts.zip", + "C:\\CI\\output", "--host-artifact-dir", str(tmp_path / "out"), "--include-logs", @@ -164,6 +195,10 @@ def test_collect_writes_manifest_when_job_id_set( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: _patch(monkeypatch) + payload = _zip_payload(tmp_path) + monkeypatch.setattr( + artifacts_module, "WinRmTransport", _win_transport_with_zip(payload) + ) out = tmp_path / "out" result = CliRunner().invoke( cli, @@ -173,7 +208,7 @@ def test_collect_writes_manifest_when_job_id_set( "--ip-address", "10.0.0.7", "--guest-artifact-path", - "C:\\CI\\output\\artifacts.zip", + "C:\\CI\\output", "--host-artifact-dir", str(out), "--job-id", @@ -189,7 +224,7 @@ def test_collect_writes_manifest_when_job_id_set( assert data["jobId"] == "run-42" assert data["commit"] == "abc1234" names = [f["name"] for f in data["files"]] - assert "artifacts.zip" in names + assert "binary.bin" in names and "log.txt" in names # ── Linux happy path ────────────────────────────────────────────────────── diff --git a/tests/python/test_commands_build.py b/tests/python/test_commands_build.py index 7978e1a..70b7700 100644 --- a/tests/python/test_commands_build.py +++ b/tests/python/test_commands_build.py @@ -360,5 +360,6 @@ def test_build_run_windows_host_source_dir( assert instance.copies[0][1] == "C:\\CI\\src-transfer.zip" cmds = [c[0] for c in instance.runs] assert any("Expand-Archive" in c for c in cmds) - # Packaging step references the configured artifact source dir. - assert any("Compress-Archive" in c and "'dist'" in c for c in cmds) + # Staging copies the raw source into the collect dir (no zip-in-zip). + assert any("Copy-Item" in c and "'dist'" in c for c in cmds) + assert not any("Compress-Archive" in c and "'dist'" in c for c in cmds) diff --git a/tests/python/test_commands_job.py b/tests/python/test_commands_job.py index ff4f9f6..1c9242a 100644 --- a/tests/python/test_commands_job.py +++ b/tests/python/test_commands_job.py @@ -322,8 +322,8 @@ def test_job_windows_branch_with_overrides( # CPU/RAM override patched the clone VMX. clone_vmx = next((tmp_path / "vms").rglob("*.vmx"), None) # After successful job the clone dir is deleted, so the file is gone. - # Check that the Windows collect helper got the correct artifact path instead. - assert calls["windows_collect"][0]["guest_path"].endswith("artifacts.zip") + # Check that the Windows collect helper got the collect dir instead. + assert calls["windows_collect"][0]["guest_path"] == "C:\\CI\\output" assert clone_vmx is None # cleaned up assert backend.calls[-1] == "delete"