From 149e8846b8f9a15ec64d3a2dbef2e0e67c9cf99c Mon Sep 17 00:00:00 2001 From: kireto Date: Wed, 26 Aug 2026 14:09:59 +0000 Subject: [PATCH] DEVX-163: fix: use stderr=STDOUT to capture all docker push output in one stream --- src/devx/tools/build_image.py | 19 +++++++------ tests/unit/test_build_image.py | 51 +++++++++++++--------------------- 2 files changed, 30 insertions(+), 40 deletions(-) diff --git a/src/devx/tools/build_image.py b/src/devx/tools/build_image.py index d04ec46..a9ee410 100644 --- a/src/devx/tools/build_image.py +++ b/src/devx/tools/build_image.py @@ -269,17 +269,19 @@ def _run_push(cmd: list[str]) -> subprocess.CompletedProcess[str]: Docker sends push progress/errors to both stdout and stderr depending on the error type, so both streams are checked for the 500 status. + Uses stderr=STDOUT to merge both streams into stdout, ensuring all + output is captured in one place (docker push output behavior varies + depending on TTY detection). """ result = subprocess.run( # nosec B603 cmd, - capture_output=True, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, text=True, check=False, ) - if result.returncode != 0: - combined = f"{result.stderr}\n{result.stdout}" - if "500" in combined: - raise PushHTTP500Error(combined.strip()) + if result.returncode != 0 and "500" in (result.stdout or ""): + raise PushHTTP500Error(result.stdout.strip()) return result @@ -334,7 +336,7 @@ def push_image( if result.returncode == 0: click.echo(f"Pushed {ft}") continue - combined_output = f"{result.stderr}\n{result.stdout}".strip() + combined_output = (result.stdout or "").strip() # Gitea #31964: push fails because tag already exists. # Delete the old manifest and retry once. if username and token and "already exists" in combined_output.lower(): @@ -350,14 +352,15 @@ def push_image( click.echo(f" Retrying push {ft}...") result = subprocess.run( # nosec B603 cmd, - capture_output=True, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, text=True, check=False, ) if result.returncode == 0: click.echo(f"Pushed {ft} (after retry)") continue - combined_output = f"{result.stderr}\n{result.stdout}".strip() + combined_output = (result.stdout or "").strip() click.echo( _("Push failed for {tag}: {error}", tag=ft, error=combined_output), err=True, diff --git a/tests/unit/test_build_image.py b/tests/unit/test_build_image.py index 91ff082..b516250 100644 --- a/tests/unit/test_build_image.py +++ b/tests/unit/test_build_image.py @@ -198,7 +198,7 @@ class TestBuildImage: class TestPushImage: def test_success(self) -> None: spec = ImageSpec(name="ci-base", dockerfile="Dockerfile", tags=["latest", "1.0"]) - mock_result = MagicMock(returncode=0, stderr="", stdout="") + mock_result = MagicMock(returncode=0, stdout="") with patch("devx.tools.build_image.subprocess.run", return_value=mock_result) as mock_run: assert push_image(spec, "git.example.com") is True assert mock_run.call_count == 2 @@ -206,8 +206,8 @@ class TestPushImage: def test_partial_failure(self) -> None: spec = ImageSpec(name="ci-base", dockerfile="Dockerfile", tags=["latest", "1.0"]) results = [ - MagicMock(returncode=0, stderr="", stdout=""), - MagicMock(returncode=1, stderr="push failed", stdout=""), + MagicMock(returncode=0, stdout=""), + MagicMock(returncode=1, stdout="push failed"), ] with patch("devx.tools.build_image.subprocess.run", side_effect=results): assert push_image(spec, "git.example.com") is False @@ -221,7 +221,7 @@ class TestPushImage: def test_no_delete_on_success_with_creds(self) -> None: """Push-first: no delete needed when push succeeds.""" spec = ImageSpec(name="ci-base", dockerfile="Dockerfile", tags=["latest"]) - mock_result = MagicMock(returncode=0, stderr="", stdout="") + mock_result = MagicMock(returncode=0, stdout="") with ( patch("devx.tools.build_image.subprocess.run", return_value=mock_result), patch("devx.tools.build_image.delete_remote_manifest") as mock_del, @@ -231,7 +231,7 @@ class TestPushImage: def test_no_delete_without_creds(self) -> None: spec = ImageSpec(name="ci-base", dockerfile="Dockerfile", tags=["latest"]) - mock_result = MagicMock(returncode=0, stderr="", stdout="") + mock_result = MagicMock(returncode=0, stdout="") with ( patch("devx.tools.build_image.subprocess.run", return_value=mock_result), patch("devx.tools.build_image.delete_remote_manifest") as mock_del, @@ -243,8 +243,8 @@ class TestPushImage: """Gitea #31964: push fails with 'already exists', delete + retry.""" spec = ImageSpec(name="ci-base", dockerfile="Dockerfile", tags=["latest"]) results = [ - MagicMock(returncode=1, stderr="package version already exists", stdout=""), - MagicMock(returncode=0, stderr="", stdout=""), + MagicMock(returncode=1, stdout="package version already exists"), + MagicMock(returncode=0, stdout=""), ] with ( patch("devx.tools.build_image.subprocess.run", side_effect=results), @@ -263,7 +263,7 @@ class TestPushImage: def test_no_delete_on_non_already_exists_failure(self) -> None: """Push fails for other reasons (non-500) — old manifest preserved.""" spec = ImageSpec(name="ci-base", dockerfile="Dockerfile", tags=["latest"]) - mock_result = MagicMock(returncode=1, stderr="denied: requested access to the resource is denied", stdout="") + mock_result = MagicMock(returncode=1, stdout="denied: requested access to the resource is denied") with ( patch("devx.tools.build_image.subprocess.run", return_value=mock_result), patch("devx.tools.build_image.delete_remote_manifest") as mock_del, @@ -275,8 +275,8 @@ class TestPushImage: """Gitea #31964 retry also fails — both pushes fail.""" spec = ImageSpec(name="ci-base", dockerfile="Dockerfile", tags=["latest"]) results = [ - MagicMock(returncode=1, stderr="package version already exists", stdout=""), - MagicMock(returncode=1, stderr="push failed again", stdout=""), + MagicMock(returncode=1, stdout="package version already exists"), + MagicMock(returncode=1, stdout="push failed again"), ] with ( patch("devx.tools.build_image.subprocess.run", side_effect=results), @@ -288,8 +288,8 @@ class TestPushImage: """HTTP 500 from registry race condition — retry succeeds.""" spec = ImageSpec(name="ci-base", dockerfile="Dockerfile", tags=["latest"]) results = [ - MagicMock(returncode=1, stderr="", stdout="received unexpected HTTP status: 500 Internal Server Error"), - MagicMock(returncode=0, stderr="", stdout=""), + MagicMock(returncode=1, stdout="received unexpected HTTP status: 500 Internal Server Error"), + MagicMock(returncode=0, stdout=""), ] with ( patch("devx.tools.build_image.subprocess.run", side_effect=results), @@ -302,9 +302,7 @@ class TestPushImage: def test_http_500_retries_all_fail(self) -> None: """HTTP 500 retries exhausted — push fails, no delete attempted.""" spec = ImageSpec(name="ci-base", dockerfile="Dockerfile", tags=["latest"]) - mock_result = MagicMock( - returncode=1, stderr="", stdout="received unexpected HTTP status: 500 Internal Server Error" - ) + mock_result = MagicMock(returncode=1, stdout="received unexpected HTTP status: 500 Internal Server Error") with ( patch("devx.tools.build_image.subprocess.run", return_value=mock_result), patch("devx.tools.build_image.delete_remote_manifest") as mock_del, @@ -313,31 +311,20 @@ class TestPushImage: assert push_image(spec, "git.example.com", username="user", token="tok") is False mock_del.assert_not_called() - def test_run_push_raises_on_500_stderr(self) -> None: - """_run_push raises PushHTTP500Error when stderr contains 500.""" + def test_run_push_raises_on_500(self) -> None: + """_run_push raises PushHTTP500Error when stdout contains 500.""" from devx.tools.build_image import _run_push - mock_result = MagicMock(returncode=1, stderr="HTTP 500 Internal Server Error", stdout="") - with patch("devx.tools.build_image.subprocess.run", return_value=mock_result): - with pytest.raises(PushHTTP500Error, match="HTTP 500"): - _run_push(["docker", "push", "img:latest"]) - - def test_run_push_raises_on_500_stdout(self) -> None: - """_run_push raises PushHTTP500Error when stdout contains 500 (docker sends to stdout).""" - from devx.tools.build_image import _run_push - - mock_result = MagicMock( - returncode=1, stderr="", stdout="received unexpected HTTP status: 500 Internal Server Error" - ) + mock_result = MagicMock(returncode=1, stdout="received unexpected HTTP status: 500 Internal Server Error") with patch("devx.tools.build_image.subprocess.run", return_value=mock_result): with pytest.raises(PushHTTP500Error, match="500"): _run_push(["docker", "push", "img:latest"]) def test_run_push_no_raise_on_non_500(self) -> None: - """_run_push returns result when stderr has no 500.""" + """_run_push returns result when stdout has no 500.""" from devx.tools.build_image import _run_push - mock_result = MagicMock(returncode=1, stderr="denied: access denied", stdout="") + mock_result = MagicMock(returncode=1, stdout="denied: access denied") with patch("devx.tools.build_image.subprocess.run", return_value=mock_result): result = _run_push(["docker", "push", "img:latest"]) assert result.returncode == 1 @@ -346,7 +333,7 @@ class TestPushImage: """_run_push returns result on success.""" from devx.tools.build_image import _run_push - mock_result = MagicMock(returncode=0, stderr="", stdout="") + mock_result = MagicMock(returncode=0, stdout="") with patch("devx.tools.build_image.subprocess.run", return_value=mock_result): result = _run_push(["docker", "push", "img:latest"]) assert result.returncode == 0