fix(cmd-publish): fix handling of asset uploading errors on publish (python-semantic-release#1397)

Resolves: python-semantic-release#1395

* fix(github): fix bubble up errors of asset uploads for GitHub

* test(cmd-publish): add e2e test for handling GitHub authentication errors on upload

* test(github): add comprehensive error tests for `upload_dists()`

Author: codejedi365

src/semantic_release/cli/commands/publish.py

@@ -6,6 +6,7 @@
 from git import Repo
 
 from semantic_release.cli.util import noop_report
+from semantic_release.errors import AssetUploadError
 from semantic_release.globals import logger
 from semantic_release.hvcs.remote_hvcs_base import RemoteHvcsBase
 from semantic_release.version.algorithm import tags_and_versions
@@ -90,9 +91,13 @@ def publish(cli_ctx: CliContextObj, tag: str) -> None:
         )
         return
 
-    publish_distributions(
-        tag=tag,
-        hvcs_client=hvcs_client,
-        dist_glob_patterns=dist_glob_patterns,
-        noop=runtime.global_cli_options.noop,
-    )
+    try:
+        publish_distributions(
+            tag=tag,
+            hvcs_client=hvcs_client,
+            dist_glob_patterns=dist_glob_patterns,
+            noop=runtime.global_cli_options.noop,
+        )
+    except AssetUploadError as err:
+        click.echo(err, err=True)
+        ctx.exit(1)

src/semantic_release/hvcs/github.py

@@ -463,14 +463,25 @@ def upload_dists(self, tag: str, dist_glob: str) -> int:
 
         # Upload assets
         n_succeeded = 0
+        errors = []
         for file_path in (
             f for f in glob.glob(dist_glob, recursive=True) if os.path.isfile(f)
         ):
             try:
                 self.upload_release_asset(release_id, file_path)
                 n_succeeded += 1
-            except HTTPError:  # noqa: PERF203
+            except HTTPError as err:  # noqa: PERF203
                 logger.exception("error uploading asset %s", file_path)
+                status_code = (
+                    err.response.status_code if err.response is not None else "unknown"
+                )
+                error_msg = f"Failed to upload asset '{file_path}' to release"
+                if status_code != "unknown":
+                    error_msg += f" (HTTP {status_code})"
+                errors.append(error_msg)
+
+        if errors:
+            raise AssetUploadError("\n".join(errors))
 
         return n_succeeded
 

tests/e2e/cmd_publish/test_publish.py

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
-from typing import TYPE_CHECKING
+from pathlib import Path
+from typing import TYPE_CHECKING, cast
 from unittest import mock
 
 import pytest
@@ -15,8 +16,15 @@
 if TYPE_CHECKING:
     from typing import Sequence
 
+    from requests_mock import Mocker
+
     from tests.conftest import RunCliFn
-    from tests.fixtures.git_repo import BuiltRepoResult, GetVersionsFromRepoBuildDefFn
+    from tests.fixtures.git_repo import (
+        BuiltRepoResult,
+        GetCfgValueFromDefFn,
+        GetHvcsClientFromRepoDefFn,
+        GetVersionsFromRepoBuildDefFn,
+    )
 
 
 @pytest.mark.parametrize("cmd_args", [(), ("--tag", "latest")])
@@ -87,3 +95,76 @@ def test_publish_fails_on_nonexistant_tag(run_cli: RunCliFn):
             f"Tag '{non_existant_tag}' not found in local repository!" in result.stderr
         )
         mocked_upload_dists.assert_not_called()
+
+
+@pytest.mark.parametrize(
+    "repo_result",
+    [
+        lazy_fixture(repo_fixture_name)
+        for repo_fixture_name in [
+            repo_w_trunk_only_conventional_commits.__name__,
+        ]
+    ],
+)
+def test_publish_fails_on_github_upload_dists(
+    repo_result: BuiltRepoResult,
+    get_hvcs_client_from_repo_def: GetHvcsClientFromRepoDefFn,
+    get_cfg_value_from_def: GetCfgValueFromDefFn,
+    get_versions_from_repo_build_def: GetVersionsFromRepoBuildDefFn,
+    run_cli: RunCliFn,
+    requests_mock: Mocker,
+):
+    """
+    Given a repo with conventional commits and at least one tag
+    When publishing to a valid tag but upload dists authentication fails
+    Then the command fails with exit code 1
+
+    Reference: python-semantic-release/publish-action#77
+    """
+    repo_def = repo_result["definition"]
+    tag_format_str = cast("str", get_cfg_value_from_def(repo_def, "tag_format_str"))
+    all_versions = get_versions_from_repo_build_def(repo_def)
+    latest_release_version = all_versions[-1]
+    release_tag = tag_format_str.format(version=latest_release_version)
+    hvcs_client = get_hvcs_client_from_repo_def(repo_def)
+    if not isinstance(hvcs_client, Github):
+        pytest.fail("Test setup error: HvcsClient is not a Github instance")
+
+    release_id = 12
+    files = [
+        Path(f"dist/package-{latest_release_version}.whl"),
+        Path(f"dist/package-{latest_release_version}.tar.gz"),
+    ]
+    tag_endpoint = hvcs_client.create_api_url(
+        endpoint=f"/repos/{hvcs_client.owner}/{hvcs_client.repo_name}/releases/tags/{release_tag}",
+    )
+    release_endpoint = hvcs_client.create_api_url(
+        endpoint=f"/repos/{hvcs_client.owner}/{hvcs_client.repo_name}/releases/{release_id}"
+    )
+    upload_url = release_endpoint + "/assets"
+    expected_num_upload_attempts = len(files)
+
+    # Setup: Create distribution files before upload
+    for file in files:
+        file.parent.mkdir(parents=True, exist_ok=True)
+        file.touch()
+
+    # Setup: Mock upload url retrieval
+    requests_mock.register_uri("GET", tag_endpoint, json={"id": release_id})
+    requests_mock.register_uri(
+        "GET", release_endpoint, json={"upload_url": f"{upload_url}{{?name,label}}"}
+    )
+
+    # Setup: Mock upload failure
+    uploader_mock = requests_mock.register_uri("POST", upload_url, status_code=403)
+
+    # Act
+    cli_cmd = [MAIN_PROG_NAME, PUBLISH_SUBCMD, "--tag", "latest"]
+    result = run_cli(cli_cmd[1:])
+
+    # Evaluate
+    assert_exit_code(1, result, cli_cmd)
+    assert isinstance(result.exception, SystemExit)
+    assert expected_num_upload_attempts == uploader_mock.call_count
+    for file in files:
+        assert f"Failed to upload asset '{file}'" in result.stderr

tests/unit/semantic_release/hvcs/test_github.py

@@ -13,6 +13,7 @@
 from requests import HTTPError, Response, Session
 from requests.auth import _basic_auth_str
 
+from semantic_release.errors import AssetUploadError
 from semantic_release.hvcs.github import Github
 from semantic_release.hvcs.token_auth import TokenAuth
 
@@ -1026,7 +1027,7 @@ def test_upload_release_asset_fails(
 
 # Note - mocking as the logic for uploading an asset
 # is covered by testing above, no point re-testing.
-def test_upload_dists_when_release_id_not_found(default_gh_client):
+def test_upload_dists_when_release_id_not_found(default_gh_client: Github):
     tag = "v1.0.0"
     path = "doesn't matter"
     expected_num_uploads = 0
@@ -1093,3 +1094,223 @@ def test_upload_dists_when_release_id_found(
         assert expected_num_uploads == num_uploads
         mock_get_release_id_by_tag.assert_called_once_with(tag=tag)
         assert expected_files_uploaded == mock_upload_release_asset.call_args_list
+
+
+@pytest.mark.parametrize(
+    "status_code, error_message",
+    [
+        (401, "Unauthorized"),
+        (403, "Forbidden"),
+        (400, "Bad Request"),
+        (404, "Not Found"),
+        (429, "Too Many Requests"),
+        (500, "Internal Server Error"),
+        (503, "Service Unavailable"),
+    ],
+)
+def test_upload_dists_fails_with_http_error(
+    default_gh_client: Github,
+    status_code: int,
+    error_message: str,
+):
+    """Given a release exists, when upload_release_asset raises HTTPError, then AssetUploadError is raised."""
+    # Setup
+    release_id = 123
+    tag = "v1.0.0"
+    files = ["dist/package-1.0.0.whl", "dist/package-1.0.0.tar.gz"]
+    glob_pattern = "dist/*"
+    expected_num_upload_attempts = len(files)
+
+    # Create mock HTTPError with proper response
+    http_error = HTTPError(error_message)
+    http_error.response = Response()
+    http_error.response.status_code = status_code
+    http_error.response._content = error_message.encode()
+
+    # Skip filesystem checks
+    mocked_isfile = mock.patch.object(os.path, "isfile", return_value=True)
+    mocked_globber = mock.patch.object(glob, "glob", return_value=files)
+
+    # Set up mock environment
+    with mocked_globber, mocked_isfile, mock.patch.object(
+        default_gh_client,
+        default_gh_client.get_release_id_by_tag.__name__,
+        return_value=release_id,
+    ) as mock_get_release_id_by_tag, mock.patch.object(
+        default_gh_client,
+        default_gh_client.upload_release_asset.__name__,
+        side_effect=http_error,
+    ) as mock_upload_release_asset:
+        # Execute method under test expecting an exception to be raised
+        with pytest.raises(AssetUploadError) as exc_info:
+            default_gh_client.upload_dists(tag, glob_pattern)
+
+        # Evaluate (expected -> actual)
+        mock_get_release_id_by_tag.assert_called_once_with(tag=tag)
+
+        # Should have attempted to upload all files even though they fail
+        assert expected_num_upload_attempts == mock_upload_release_asset.call_count
+
+        # Verify the error message contains useful information about failed uploads
+        error_msg = str(exc_info.value)
+
+        # Each file should be mentioned in the error message with status code
+        for file in files:
+            assert f"Failed to upload asset '{file}'" in error_msg
+            assert f"(HTTP {status_code})" in error_msg
+
+
+def test_upload_dists_fails_authentication_error_401(default_gh_client: Github):
+    """Given a release exists, when upload fails with 401, then AssetUploadError is raised with auth context."""
+    # Setup
+    release_id = 456
+    tag = "v2.0.0"
+    files = ["dist/package-2.0.0.whl"]
+    glob_pattern = "dist/*.whl"
+
+    # Create mock HTTPError for authentication failure
+    http_error = HTTPError("401 Client Error: Unauthorized")
+    http_error.response = Response()
+    http_error.response.status_code = 401
+    http_error.response._content = b'{"message": "Bad credentials"}'
+
+    # Skip filesystem checks
+    mocked_isfile = mock.patch.object(os.path, "isfile", return_value=True)
+    mocked_globber = mock.patch.object(glob, "glob", return_value=files)
+
+    # Set up mock environment
+    with mocked_globber, mocked_isfile, mock.patch.object(
+        default_gh_client,
+        default_gh_client.get_release_id_by_tag.__name__,
+        return_value=release_id,
+    ), mock.patch.object(
+        default_gh_client,
+        default_gh_client.upload_release_asset.__name__,
+        side_effect=http_error,
+    ):
+        # Execute method under test expecting an exception to be raised
+        with pytest.raises(AssetUploadError) as exc_info:
+            default_gh_client.upload_dists(tag, glob_pattern)
+
+        # Verify the error message contains file, release information and status code
+        error_msg = str(exc_info.value)
+        assert "Failed to upload asset" in error_msg
+        assert files[0] in error_msg
+        assert "(HTTP 401)" in error_msg
+
+
+def test_upload_dists_fails_forbidden_error_403(default_gh_client: Github):
+    """Given a release exists, when upload fails with 403, then AssetUploadError is raised with permission context."""
+    # Setup
+    release_id = 789
+    tag = "v3.0.0"
+    files = ["dist/package-3.0.0.tar.gz"]
+    glob_pattern = "dist/*.tar.gz"
+
+    # Create mock HTTPError for forbidden access
+    http_error = HTTPError("403 Client Error: Forbidden")
+    http_error.response = Response()
+    http_error.response.status_code = 403
+
+    # Skip filesystem checks
+    mocked_isfile = mock.patch.object(os.path, "isfile", return_value=True)
+    mocked_globber = mock.patch.object(glob, "glob", return_value=files)
+
+    # Set up mock environment
+    with mocked_globber, mocked_isfile, mock.patch.object(
+        default_gh_client,
+        default_gh_client.get_release_id_by_tag.__name__,
+        return_value=release_id,
+    ), mock.patch.object(
+        default_gh_client,
+        default_gh_client.upload_release_asset.__name__,
+        side_effect=http_error,
+    ):
+        # Execute method under test expecting an exception to be raised
+        with pytest.raises(AssetUploadError) as exc_info:
+            default_gh_client.upload_dists(tag, glob_pattern)
+
+        # Verify the error message contains file, release information and status code
+        error_msg = str(exc_info.value)
+        assert "Failed to upload asset" in error_msg
+        assert f"Failed to upload asset '{files[0]}'" in error_msg
+        assert "(HTTP 403)" in error_msg
+
+
+def test_upload_dists_partial_failure(default_gh_client: Github):
+    """Given multiple files to upload, when some succeed and some fail, then AssetUploadError is raised."""
+    # Setup
+    release_id = 999
+    tag = "v4.0.0"
+    files = [
+        "dist/package-4.0.0.whl",
+        "dist/package-4.0.0.tar.gz",
+        "dist/package-4.0.0-py3-none-any.whl",
+    ]
+    glob_pattern = "dist/*"
+    expected_num_upload_attempts = len(files)
+
+    # Create mock HTTPError for the second file
+    http_error = HTTPError("500 Server Error: Internal Server Error")
+    http_error.response = Response()
+    http_error.response.status_code = 500
+
+    # Skip filesystem checks
+    mocked_isfile = mock.patch.object(os.path, "isfile", return_value=True)
+    mocked_globber = mock.patch.object(glob, "glob", return_value=files)
+
+    # Set up mock environment - first upload succeeds, second fails, third fails
+    upload_results = [True, http_error, http_error]
+
+    with mocked_globber, mocked_isfile, mock.patch.object(
+        default_gh_client,
+        default_gh_client.get_release_id_by_tag.__name__,
+        return_value=release_id,
+    ), mock.patch.object(
+        default_gh_client,
+        default_gh_client.upload_release_asset.__name__,
+        side_effect=upload_results,
+    ) as mock_upload_release_asset:
+        # Execute method under test expecting an exception to be raised
+        with pytest.raises(AssetUploadError) as exc_info:
+            default_gh_client.upload_dists(tag, glob_pattern)
+
+        # Verify all uploads were attempted
+        assert expected_num_upload_attempts == mock_upload_release_asset.call_count
+
+        # Verify the error message mentions the failed files with status code
+        error_msg = str(exc_info.value)
+        assert f"Failed to upload asset '{files[1]}'" in error_msg
+        assert f"Failed to upload asset '{files[2]}'" in error_msg
+        assert "(HTTP 500)" in error_msg
+
+
+def test_upload_dists_all_succeed(default_gh_client: Github):
+    """Given multiple files to upload, when all succeed, then return count of successful uploads."""
+    # Setup
+    release_id = 111
+    tag = "v5.0.0"
+    files = ["dist/package-5.0.0.whl", "dist/package-5.0.0.tar.gz"]
+    glob_pattern = "dist/*"
+    expected_num_uploads = len(files)
+
+    # Skip filesystem checks
+    mocked_isfile = mock.patch.object(os.path, "isfile", return_value=True)
+    mocked_globber = mock.patch.object(glob, "glob", return_value=files)
+
+    # Set up mock environment - all uploads succeed
+    with mocked_globber, mocked_isfile, mock.patch.object(
+        default_gh_client,
+        default_gh_client.get_release_id_by_tag.__name__,
+        return_value=release_id,
+    ), mock.patch.object(
+        default_gh_client,
+        default_gh_client.upload_release_asset.__name__,
+        return_value=True,
+    ) as mock_upload_release_asset:
+        # Execute method under test
+        num_uploads = default_gh_client.upload_dists(tag, glob_pattern)
+
+        # Evaluate (expected -> actual)
+        assert expected_num_uploads == num_uploads
+        assert expected_num_uploads == mock_upload_release_asset.call_count