diff --git a/src/semantic_release/cli/commands/publish.py b/src/semantic_release/cli/commands/publish.py index 4efab72de..afcd24d2f 100644 --- a/src/semantic_release/cli/commands/publish.py +++ b/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) diff --git a/src/semantic_release/hvcs/github.py b/src/semantic_release/hvcs/github.py index 5ccc9004b..b2c9e5be4 100644 --- a/src/semantic_release/hvcs/github.py +++ b/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 diff --git a/tests/e2e/cmd_publish/test_publish.py b/tests/e2e/cmd_publish/test_publish.py index 3b4fca2bf..23eb89bb0 100644 --- a/tests/e2e/cmd_publish/test_publish.py +++ b/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 diff --git a/tests/unit/semantic_release/hvcs/test_github.py b/tests/unit/semantic_release/hvcs/test_github.py index e7f69a5ea..48f0564f5 100644 --- a/tests/unit/semantic_release/hvcs/test_github.py +++ b/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