diff --git a/docs/specs/DEVX-159.md b/docs/specs/DEVX-159.md new file mode 100644 index 0000000..f5b0550 --- /dev/null +++ b/docs/specs/DEVX-159.md @@ -0,0 +1,41 @@ +# DEVX-159: Fix build_image push-first strategy to avoid losing latest tag + +## Problem +The `push_image` function in `build_image.py` deletes the existing +manifest *before* pushing (Gitea #31964 workaround). When the push +fails for other reasons (HTTP 500), the old tag is lost, breaking all +CI jobs that use that image. + +This caused `ci-base:latest` to disappear from the registry when +build-images run #4104 failed with HTTP 500 on push, after already +deleting the old `latest` manifest. + +## Approach +Switch to a push-first strategy: +1. Try pushing directly +2. Only if push fails with "already exists" (Gitea #31964), delete + the old manifest and retry +3. If push fails for any other reason, the old manifest is preserved + +REQ-1: Push first, no pre-emptive delete +REQ-2: Delete + retry only on "already exists" error +REQ-3: Old manifest preserved on non-already-exists failures +REQ-4: 100% test coverage of new logic + +## Test Plan +- Unit tests for all push paths (success, already-exists retry, + non-already-exists failure, retry-also-fails) +- Verify existing tests still pass + +## Deploy Plan +- Merge to master, build-images workflow uses new push logic on next + image rebuild + +## Rollback Plan +- Revert the merge commit + +## Acceptance Criteria +- [x] REQ-1: Push first, no pre-emptive delete +- [x] REQ-2: Delete + retry only on "already exists" error +- [x] REQ-3: Old manifest preserved on non-already-exists failures +- [x] REQ-4: 100% test coverage of new logic diff --git a/src/devx/tools/build_image.py b/src/devx/tools/build_image.py index 3a767c9..fad3e90 100644 --- a/src/devx/tools/build_image.py +++ b/src/devx/tools/build_image.py @@ -265,20 +265,15 @@ def push_image( """Push all tags of a Docker image to the registry. Returns True if all pushes succeed, False if any fail. + + Push-first strategy: try pushing directly. Only if the push fails + with Gitea #31964 ("package version already exists") do we delete + the old manifest and retry. This avoids losing the existing tag + when the push fails for unrelated reasons (e.g. HTTP 500). """ full_tags = [build_full_tag(registry, spec.name, t) for t in spec.tags] all_ok = True for ft, tag in zip(full_tags, spec.tags, strict=False): - # Workaround for Gitea #31964: delete existing tag before push - if username and token: - delete_remote_manifest( - registry, - spec.name, - tag, - username, - token, - dry_run=dry_run, - ) cmd = ["docker", "push", ft] if dry_run: click.echo(f"[dry-run] {' '.join(cmd)}") @@ -290,14 +285,38 @@ def push_image( text=True, check=False, ) - if result.returncode != 0: - click.echo( - _("Push failed for {tag}: {error}", tag=ft, error=result.stderr.strip()), - err=True, - ) - all_ok = False - else: + if result.returncode == 0: click.echo(f"Pushed {ft}") + continue + stderr = result.stderr.strip() + # Gitea #31964: push fails because tag already exists. + # Delete the old manifest and retry once. + if username and token and "already exists" in stderr.lower(): + click.echo(" Tag exists (Gitea #31964), deleting old manifest and retrying...") + delete_remote_manifest( + registry, + spec.name, + tag, + username, + token, + dry_run=dry_run, + ) + click.echo(f" Retrying push {ft}...") + result = subprocess.run( # nosec B603 + cmd, + capture_output=True, + text=True, + check=False, + ) + if result.returncode == 0: + click.echo(f"Pushed {ft} (after retry)") + continue + stderr = result.stderr.strip() + click.echo( + _("Push failed for {tag}: {error}", tag=ft, error=stderr), + err=True, + ) + all_ok = False return all_ok diff --git a/tests/unit/test_build_image.py b/tests/unit/test_build_image.py index 2d92c3f..ba19b71 100644 --- a/tests/unit/test_build_image.py +++ b/tests/unit/test_build_image.py @@ -217,11 +217,36 @@ class TestPushImage: assert push_image(spec, "git.example.com", dry_run=True) is True mock_run.assert_not_called() - def test_delete_before_push_with_creds(self) -> None: + 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="") with ( patch("devx.tools.build_image.subprocess.run", return_value=mock_result), + patch("devx.tools.build_image.delete_remote_manifest") as mock_del, + ): + assert push_image(spec, "git.example.com", username="user", token="tok") is True + mock_del.assert_not_called() + + def test_no_delete_without_creds(self) -> None: + spec = ImageSpec(name="ci-base", dockerfile="Dockerfile", tags=["latest"]) + mock_result = MagicMock(returncode=0, stderr="", stdout="") + with ( + patch("devx.tools.build_image.subprocess.run", return_value=mock_result), + patch("devx.tools.build_image.delete_remote_manifest") as mock_del, + ): + assert push_image(spec, "git.example.com") is True + mock_del.assert_not_called() + + def test_delete_and_retry_on_already_exists(self) -> None: + """Gitea #31964: push fails with 'already exists', delete + retry.""" + spec = ImageSpec(name="ci-base", dockerfile="Dockerfile", tags=["latest"]) + results = [ + MagicMock(returncode=1, stderr="500 Internal Server Error: already exists", stdout=""), + MagicMock(returncode=0, stderr="", stdout=""), + ] + with ( + patch("devx.tools.build_image.subprocess.run", side_effect=results), patch("devx.tools.build_image.delete_remote_manifest", return_value=True) as mock_del, ): assert push_image(spec, "git.example.com", username="user", token="tok") is True @@ -234,16 +259,32 @@ class TestPushImage: dry_run=False, ) - def test_no_delete_without_creds(self) -> None: + def test_no_delete_on_non_already_exists_failure(self) -> None: + """Push fails for other reasons (HTTP 500) — old manifest preserved.""" spec = ImageSpec(name="ci-base", dockerfile="Dockerfile", tags=["latest"]) - mock_result = MagicMock(returncode=0, stderr="", stdout="") + mock_result = MagicMock( + returncode=1, stderr="received unexpected HTTP status: 500 Internal Server Error", stdout="" + ) with ( patch("devx.tools.build_image.subprocess.run", return_value=mock_result), patch("devx.tools.build_image.delete_remote_manifest") as mock_del, ): - assert push_image(spec, "git.example.com") is True + assert push_image(spec, "git.example.com", username="user", token="tok") is False mock_del.assert_not_called() + def test_retry_also_fails(self) -> None: + """Gitea #31964 retry also fails — both pushes fail.""" + spec = ImageSpec(name="ci-base", dockerfile="Dockerfile", tags=["latest"]) + results = [ + MagicMock(returncode=1, stderr="500 Internal Server Error: already exists", stdout=""), + MagicMock(returncode=1, stderr="push failed again", stdout=""), + ] + with ( + patch("devx.tools.build_image.subprocess.run", side_effect=results), + patch("devx.tools.build_image.delete_remote_manifest", return_value=True), + ): + assert push_image(spec, "git.example.com", username="user", token="tok") is False + class TestDeleteRemoteManifest: def test_dry_run(self) -> None: