diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index af5af197..2487faeb 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -90,6 +90,28 @@ async def _checkout_worktree( repo_dir = self.github_webhook.clone_repo_dir self.logger.debug(f"{self.log_prefix} Creating worktree from {repo_dir} with checkout: {checkout_target}") + # Check if checkout_target is already checked out in main clone + # This prevents "already used by worktree" error when target branch matches current branch + rc, current_branch, _ = await run_command( + command=f"git -C {repo_dir} rev-parse --abbrev-ref HEAD", + log_prefix=self.log_prefix, + mask_sensitive=self.github_webhook.mask_sensitive, + ) + + if rc and current_branch.strip(): + current = current_branch.strip() + # Normalize checkout_target (remove origin/ prefix if present) + target = checkout_target.replace("origin/", "") + + if current == target: + # Current branch matches target - use main clone directly + self.logger.debug( + f"{self.log_prefix} Branch {target} already checked out in main clone, " + "using main clone instead of worktree" + ) + yield (True, repo_dir, "", "") + return + # Create worktree for this operation async with helpers_module.git_worktree_checkout( repo_dir=repo_dir, @@ -366,7 +388,8 @@ async def run_build_container( ) output["text"] = self.check_run_handler.get_check_run_text(out=out, err=err) if pull_request and set_check: - return await self.check_run_handler.set_container_build_failure(output=output) + await self.check_run_handler.set_container_build_failure(output=output) + return self.logger.step( # type: ignore[attr-defined] f"{self.log_prefix} {format_task_fields('runner', 'ci_check', 'processing')} " diff --git a/webhook_server/tests/test_container_utils.py b/webhook_server/tests/test_container_utils.py deleted file mode 100644 index 268c5ce9..00000000 --- a/webhook_server/tests/test_container_utils.py +++ /dev/null @@ -1,266 +0,0 @@ -"""Tests for container_utils module.""" - -from unittest.mock import Mock - -import pytest -from github.PullRequest import PullRequest - -from webhook_server.utils.container_utils import get_container_repository_and_tag - - -class TestGetContainerRepositoryAndTag: - """Test suite for get_container_repository_and_tag function.""" - - @pytest.fixture - def mock_logger(self) -> Mock: - """Create a mock logger.""" - return Mock() - - @pytest.fixture - def mock_pull_request(self) -> Mock: - """Create a mock PyGithub PullRequest.""" - pr = Mock(spec=PullRequest) - pr.number = 123 - pr.base = Mock() - pr.base.ref = "main" - return pr - - def test_explicit_tag_provided(self, mock_logger: Mock) -> None: - """Test with explicit tag provided.""" - result = get_container_repository_and_tag( - container_repository="quay.io/myorg/myimage", - container_tag="latest", - tag="v1.2.3", - logger=mock_logger, - log_prefix="[TEST]", - ) - - assert result == "quay.io/myorg/myimage:v1.2.3" - mock_logger.debug.assert_called_once_with("[TEST] container tag is: v1.2.3") - - def test_explicit_tag_with_hash(self, mock_logger: Mock) -> None: - """Test with hash-based tag.""" - result = get_container_repository_and_tag( - container_repository="docker.io/myorg/myimage", - container_tag="latest", - tag="abc123def456", # pragma: allowlist secret - logger=mock_logger, - ) - - assert result == "docker.io/myorg/myimage:abc123def456" - - def test_merged_pr_main_branch(self, mock_pull_request: Mock, mock_logger: Mock) -> None: - """Test merged PR on main branch uses default container tag.""" - mock_pull_request.base.ref = "main" - - result = get_container_repository_and_tag( - container_repository="ghcr.io/myorg/myimage", - container_tag="latest", - is_merged=True, - pull_request=mock_pull_request, - logger=mock_logger, - ) - - assert result == "ghcr.io/myorg/myimage:latest" - - def test_merged_pr_master_branch(self, mock_pull_request: Mock, mock_logger: Mock) -> None: - """Test merged PR on master branch uses default container tag.""" - mock_pull_request.base.ref = "master" - - result = get_container_repository_and_tag( - container_repository="quay.io/myorg/myimage", - container_tag="stable", - is_merged=True, - pull_request=mock_pull_request, - logger=mock_logger, - ) - - assert result == "quay.io/myorg/myimage:stable" - - def test_merged_pr_feature_branch(self, mock_pull_request: Mock, mock_logger: Mock) -> None: - """Test merged PR on feature branch uses branch name as tag.""" - mock_pull_request.base.ref = "feature/new-api" - - result = get_container_repository_and_tag( - container_repository="quay.io/myorg/myimage", - container_tag="latest", - is_merged=True, - pull_request=mock_pull_request, - logger=mock_logger, - ) - - assert result == "quay.io/myorg/myimage:feature/new-api" - - def test_merged_pr_release_branch(self, mock_pull_request: Mock, mock_logger: Mock) -> None: - """Test merged PR on release branch uses branch name as tag.""" - mock_pull_request.base.ref = "release-v2.0" - - result = get_container_repository_and_tag( - container_repository="quay.io/myorg/myimage", - container_tag="latest", - is_merged=True, - pull_request=mock_pull_request, - logger=mock_logger, - ) - - assert result == "quay.io/myorg/myimage:release-v2.0" - - def test_unmerged_pr(self, mock_pull_request: Mock, mock_logger: Mock) -> None: - """Test unmerged PR uses pr-{number} tag format.""" - mock_pull_request.number = 456 - - result = get_container_repository_and_tag( - container_repository="quay.io/myorg/myimage", - container_tag="latest", - is_merged=False, - pull_request=mock_pull_request, - logger=mock_logger, - ) - - assert result == "quay.io/myorg/myimage:pr-456" - - def test_no_tag_no_pull_request(self, mock_logger: Mock) -> None: - """Test returns None when no tag and no PR provided.""" - result = get_container_repository_and_tag( - container_repository="quay.io/myorg/myimage", - container_tag="latest", - logger=mock_logger, - log_prefix="[ERROR]", - ) - - assert result is None - mock_logger.error.assert_called_once_with("[ERROR] No pull request provided and no tag specified") - - def test_repository_with_port(self, mock_logger: Mock) -> None: - """Test repository URL with port number.""" - result = get_container_repository_and_tag( - container_repository="registry.example.com:5000/myorg/myimage", - container_tag="latest", - tag="v2.0.0", - logger=mock_logger, - ) - - assert result == "registry.example.com:5000/myorg/myimage:v2.0.0" - - def test_repository_with_nested_path(self, mock_logger: Mock) -> None: - """Test repository with nested path.""" - result = get_container_repository_and_tag( - container_repository="quay.io/myorg/team/myimage", - container_tag="latest", - tag="dev", - logger=mock_logger, - ) - - assert result == "quay.io/myorg/team/myimage:dev" - - def test_tag_with_special_characters(self, mock_logger: Mock) -> None: - """Test tag with special characters like dots and hyphens.""" - result = get_container_repository_and_tag( - container_repository="quay.io/myorg/myimage", - container_tag="latest", - tag="v1.2.3-rc.1", - logger=mock_logger, - ) - - assert result == "quay.io/myorg/myimage:v1.2.3-rc.1" - - def test_without_logger(self) -> None: - """Test function works without logger (logger is optional).""" - result = get_container_repository_and_tag( - container_repository="quay.io/myorg/myimage", - container_tag="latest", - tag="v1.0.0", - ) - - assert result == "quay.io/myorg/myimage:v1.0.0" - - def test_without_logger_no_tag_no_pr(self) -> None: - """Test returns None without logger when no tag and no PR.""" - result = get_container_repository_and_tag( - container_repository="quay.io/myorg/myimage", - container_tag="latest", - ) - - assert result is None - - def test_without_log_prefix(self, mock_logger: Mock) -> None: - """Test function works without log_prefix (uses empty string by default).""" - result = get_container_repository_and_tag( - container_repository="quay.io/myorg/myimage", - container_tag="latest", - tag="test", - logger=mock_logger, - ) - - assert result == "quay.io/myorg/myimage:test" - mock_logger.debug.assert_called_once_with(" container tag is: test") - - def test_pr_number_zero(self, mock_pull_request: Mock, mock_logger: Mock) -> None: - """Test PR with number 0 (edge case).""" - mock_pull_request.number = 0 - - result = get_container_repository_and_tag( - container_repository="quay.io/myorg/myimage", - container_tag="latest", - is_merged=False, - pull_request=mock_pull_request, - logger=mock_logger, - ) - - assert result == "quay.io/myorg/myimage:pr-0" - - def test_very_long_branch_name(self, mock_pull_request: Mock, mock_logger: Mock) -> None: - """Test with very long branch name.""" - long_branch = "feature/" + "x" * 100 - mock_pull_request.base.ref = long_branch - - result = get_container_repository_and_tag( - container_repository="quay.io/myorg/myimage", - container_tag="latest", - is_merged=True, - pull_request=mock_pull_request, - logger=mock_logger, - ) - - assert result == f"quay.io/myorg/myimage:{long_branch}" - - def test_empty_container_repository(self, mock_logger: Mock) -> None: - """Test with empty container repository string.""" - result = get_container_repository_and_tag( - container_repository="", - container_tag="latest", - tag="v1.0.0", - logger=mock_logger, - ) - - assert result == ":v1.0.0" - - def test_empty_tag_string(self, mock_pull_request: Mock, mock_logger: Mock) -> None: - """Test with explicitly empty tag string (should use PR logic).""" - result = get_container_repository_and_tag( - container_repository="quay.io/myorg/myimage", - container_tag="latest", - tag="", - is_merged=False, - pull_request=mock_pull_request, - logger=mock_logger, - ) - - assert result == "quay.io/myorg/myimage:pr-123" - - def test_merged_pr_main_with_empty_container_tag(self, mock_pull_request: Mock, mock_logger: Mock) -> None: - """Test merged PR on main with empty default container tag.""" - mock_pull_request.base.ref = "main" - - result = get_container_repository_and_tag( - container_repository="quay.io/myorg/myimage", - container_tag="", # Empty default tag - is_merged=True, - pull_request=mock_pull_request, - logger=mock_logger, - ) - - # When merged to main with empty container_tag, tag becomes empty string - # This triggers the final error path - assert result is None - mock_logger.error.assert_called_with(" container tag not found") diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index 4919dba7..450daaee 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -896,3 +896,116 @@ async def test_cherry_pick_manual_needed(self, runner_handler, mock_pull_request mock_set_progress.assert_called_once() mock_set_failure.assert_called_once() mock_comment.assert_called_once() + + @pytest.mark.asyncio + async def test_checkout_worktree_branch_already_checked_out( + self, runner_handler: RunnerHandler, mock_pull_request: Mock + ) -> None: + """Test _checkout_worktree when target branch is already checked out in main clone. + + This tests the worktree conflict fix - when the target branch is already + checked out in the main clone, use the main clone instead of creating a worktree. + """ + # Mock git command to return current branch as "main" + with patch( + "webhook_server.libs.handlers.runner_handler.run_command", + new=AsyncMock(return_value=(True, "main\n", "")), + ): + # Pass checkout="main" which matches the current branch + async with runner_handler._checkout_worktree(pull_request=mock_pull_request, checkout="main") as result: + success, worktree_path, out, err = result + assert success is True + # Should use main clone directory instead of creating worktree + assert worktree_path == runner_handler.github_webhook.clone_repo_dir + assert out == "" + assert err == "" + + @pytest.mark.asyncio + async def test_checkout_worktree_branch_already_checked_out_with_origin_prefix( + self, runner_handler: RunnerHandler, mock_pull_request: Mock + ) -> None: + """Test _checkout_worktree when target branch with origin/ prefix matches current branch. + + This tests the normalization logic that strips origin/ prefix from checkout target. + """ + # Mock git command to return current branch as "main" + with patch( + "webhook_server.libs.handlers.runner_handler.run_command", + new=AsyncMock(return_value=(True, "main\n", "")), + ): + # Pass checkout="origin/main" which normalizes to "main" and matches current branch + async with runner_handler._checkout_worktree( + pull_request=mock_pull_request, checkout="origin/main" + ) as result: + success, worktree_path, out, err = result + assert success is True + # Should use main clone directory instead of creating worktree + assert worktree_path == runner_handler.github_webhook.clone_repo_dir + assert out == "" + assert err == "" + + @pytest.mark.asyncio + async def test_checkout_worktree_different_branch( + self, runner_handler: RunnerHandler, mock_pull_request: Mock + ) -> None: + """Test _checkout_worktree when target branch differs from current branch. + + This tests that a worktree is created when branches don't match. + """ + # Mock git command to return current branch as "main" + with patch( + "webhook_server.libs.handlers.runner_handler.run_command", + new=AsyncMock(return_value=(True, "main\n", "")), + ): + with patch("webhook_server.utils.helpers.git_worktree_checkout") as mock_git_worktree: + mock_git_worktree.return_value.__aenter__ = AsyncMock( + return_value=(True, "/tmp/worktree-path", "success", "") + ) + mock_git_worktree.return_value.__aexit__ = AsyncMock(return_value=None) + # Pass checkout="feature-branch" which differs from current "main" + async with runner_handler._checkout_worktree( + pull_request=mock_pull_request, checkout="feature-branch" + ) as result: + success, worktree_path, _, _ = result + assert success is True + # Should create a worktree, not use main clone + assert worktree_path == "/tmp/worktree-path" + # Verify git_worktree_checkout was called + mock_git_worktree.assert_called_once() + + @pytest.mark.asyncio + async def test_run_build_container_prepare_failure( + self, runner_handler: RunnerHandler, mock_pull_request: Mock + ) -> None: + """Test run_build_container returns early when repository preparation fails. + + This tests the early return logic added in lines 385-392. + """ + runner_handler.github_webhook.pypi = {"token": "dummy"} + with patch.object( + runner_handler.github_webhook, "container_repository_and_tag", return_value="test/repo:latest" + ): + with patch.object( + runner_handler.check_run_handler, "is_check_run_in_progress", new=AsyncMock(return_value=False) + ): + with patch.object( + runner_handler.check_run_handler, "set_container_build_in_progress", new=AsyncMock() + ) as mock_set_progress: + with patch.object( + runner_handler.check_run_handler, "set_container_build_failure", new=AsyncMock() + ) as mock_set_failure: + with patch.object(runner_handler, "_checkout_worktree") as mock_checkout: + # Repository preparation fails + mock_checkout.return_value = AsyncMock() + mock_checkout.return_value.__aenter__ = AsyncMock( + return_value=(False, "/tmp/worktree-path", "checkout failed", "checkout error") + ) + mock_checkout.return_value.__aexit__ = AsyncMock(return_value=None) + with patch.object(runner_handler, "run_podman_command", new=AsyncMock()) as mock_run_podman: + await runner_handler.run_build_container(pull_request=mock_pull_request) + # Should set in progress + mock_set_progress.assert_awaited_once() + # Should set failure due to repo preparation failure + mock_set_failure.assert_awaited_once() + # Should NOT call run_podman_command (early return) + mock_run_podman.assert_not_called() diff --git a/webhook_server/utils/container_utils.py b/webhook_server/utils/container_utils.py deleted file mode 100644 index d928ceef..00000000 --- a/webhook_server/utils/container_utils.py +++ /dev/null @@ -1,55 +0,0 @@ -"""Container build utilities.""" - -from __future__ import annotations - -from logging import Logger - -from github.PullRequest import PullRequest - -from webhook_server.utils.constants import OTHER_MAIN_BRANCH - - -def get_container_repository_and_tag( - container_repository: str, - container_tag: str, - is_merged: bool = False, - tag: str = "", - pull_request: PullRequest | None = None, - logger: Logger | None = None, - log_prefix: str = "", -) -> str | None: - """ - Get container repository and tag for build. - - Args: - container_repository: Base container repository URL - container_tag: Default tag to use - is_merged: Whether PR is merged - tag: Optional explicit tag override - pull_request: Pull request object (PyGithub PullRequest, needed if tag not provided) - logger: Logger instance for debug output - log_prefix: Prefix for log messages - - Returns: - Full container repository:tag string, or None if tag cannot be determined - """ - if not tag: - if not pull_request: - if logger: - logger.error(f"{log_prefix} No pull request provided and no tag specified") - return None - - if is_merged: - pull_request_branch = pull_request.base.ref - tag = pull_request_branch if pull_request_branch not in (OTHER_MAIN_BRANCH, "main") else container_tag - else: - tag = f"pr-{pull_request.number}" - - if tag: - if logger: - logger.debug(f"{log_prefix} container tag is: {tag}") - return f"{container_repository}:{tag}" - - if logger: - logger.error(f"{log_prefix} container tag not found") - return None