Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ fix = true
output-format = "grouped"

[tool.ruff.lint]
select = ["E", "F", "W", "I", "B", "UP", "PLC0415", "ARG"]
select = ["E", "F", "W", "I", "B", "UP", "PLC0415", "ARG", "RUF059"]

[tool.ruff.lint.per-file-ignores]
"webhook_server/tests/*" = ["ARG"]
Expand Down
4 changes: 2 additions & 2 deletions webhook_server/tests/test_edge_cases_validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -548,9 +548,9 @@ def test_multiple_filter_combinations(self):
assert isinstance(filtered, list)
# Verify all filter conditions are satisfied
for entry in filtered:
if "level" in filter_params and filter_params["level"]:
if filter_params.get("level"):
assert entry.level == filter_params["level"]
if "repository" in filter_params and filter_params["repository"]:
if filter_params.get("repository"):
assert entry.repository == filter_params["repository"]


Expand Down
25 changes: 20 additions & 5 deletions webhook_server/tests/test_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -316,18 +316,33 @@ def exception(self):
def log(self, msg):
self.logged = msg

# Exception result
# Exception result - result() should RAISE the exception
class DummyFutureException:
def result(self):
return (False, "fail", lambda msg: self.log(msg))
raise RuntimeError("Repository configuration crashed")

def exception(self):
return Exception("fail-exc")
return RuntimeError("Repository configuration crashed")

def log(self, msg):
self.logged = msg

futures = [DummyFuture(), DummyFutureFail(), DummyFutureException()]
# Patch as_completed to just yield the futures

# Patch as_completed to just yield the futures and capture logger calls
with patch("webhook_server.utils.helpers.as_completed", return_value=futures):
get_future_results(futures)
with patch("webhook_server.utils.helpers.get_logger_with_params") as mock_get_logger:
mock_logger = Mock()
mock_get_logger.return_value = mock_logger

get_future_results(futures)

# Verify logger.exception was called for the exception case
mock_logger.exception.assert_called_once_with(
"Repository configuration crashed. Check for archived repositories or API permission issues."
)

# Verify all futures were processed (success and failure futures should have logged)
assert futures[0].logged == "success"
assert futures[1].logged == "fail"
# futures[2] raised exception so no log attribute set
4 changes: 2 additions & 2 deletions webhook_server/tests/test_helpers_sanitization.py
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ async def test_run_command_mask_sensitive_false(self) -> None:
mock_get_logger.return_value = mock_logger

# Run command with a secret and mask_sensitive=False
success, stdout, stderr = await run_command(
success, stdout, _ = await run_command(
command="echo 'token: ghp_test123'",
log_prefix="test",
redact_secrets=["ghp_test123"],
Expand Down Expand Up @@ -411,7 +411,7 @@ async def test_run_command_mask_sensitive_true(self) -> None:
mock_get_logger.return_value = mock_logger

# Run command with a secret and mask_sensitive=True
success, stdout, stderr = await run_command(
success, stdout, _ = await run_command(
command="echo 'token: ghp_test456'",
log_prefix="test",
redact_secrets=["ghp_test456"],
Expand Down
24 changes: 12 additions & 12 deletions webhook_server/tests/test_runner_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ async def test_run_podman_command_success(self, runner_handler: RunnerHandler) -
with patch(
"webhook_server.libs.handlers.runner_handler.run_command", new=AsyncMock(return_value=(True, "success", ""))
):
rc, out, err = await runner_handler.run_podman_command("podman build .")
rc, out, _ = await runner_handler.run_podman_command("podman build .")
assert rc is True
assert "success" in out # Relaxed assertion

Expand All @@ -110,7 +110,7 @@ async def test_run_podman_command_podman_bug(self, runner_handler: RunnerHandler
with patch("webhook_server.libs.handlers.runner_handler.run_command", new=AsyncMock()) as mock_run:
mock_run.side_effect = [(False, "output", podman_bug_err), (True, "success after fix", "")]
with patch.object(runner_handler, "fix_podman_bug") as mock_fix:
rc, out, err = await runner_handler.run_podman_command("podman build .")
_, _, _ = await runner_handler.run_podman_command("podman build .")
assert mock_fix.call_count >= 1

@pytest.mark.asyncio
Expand All @@ -120,7 +120,7 @@ async def test_run_podman_command_other_error(self, runner_handler: RunnerHandle
"webhook_server.libs.handlers.runner_handler.run_command",
new=AsyncMock(return_value=(False, "output", "other error")),
):
rc, out, err = await runner_handler.run_podman_command("podman build .")
rc, _, _ = await runner_handler.run_podman_command("podman build .")
assert rc is False or rc is None

@pytest.mark.asyncio
Expand Down Expand Up @@ -510,7 +510,7 @@ async def test_prepare_cloned_repo_dir_success(
async with runner_handler._prepare_cloned_repo_dir(
"/tmp/test-repo-unique", mock_pull_request
) as result:
success, out, err = result
success, _, _ = result
assert success is True

@pytest.mark.asyncio
Expand All @@ -521,7 +521,7 @@ async def test_prepare_cloned_repo_dir_clone_failure(self, runner_handler: Runne
new=AsyncMock(return_value=(False, "output", "error")),
):
async with runner_handler._prepare_cloned_repo_dir("/tmp/test-repo-unique2") as result:
success, out, err = result
success, out, _ = result
assert success is False
assert out == "output"

Expand All @@ -536,7 +536,7 @@ async def test_prepare_cloned_repo_dir_with_checkout(
async with runner_handler._prepare_cloned_repo_dir(
"/tmp/test-repo-unique3", mock_pull_request, checkout="feature-branch"
) as result:
success, out, err = result
success, _, _ = result
assert success is True

@pytest.mark.asyncio
Expand All @@ -550,7 +550,7 @@ async def test_prepare_cloned_repo_dir_with_tag(
async with runner_handler._prepare_cloned_repo_dir(
"/tmp/test-repo-unique4", mock_pull_request, tag_name="v1.0.0"
) as result:
success, out, err = result
success, _, _ = result
assert success is True

@pytest.mark.asyncio
Expand All @@ -564,7 +564,7 @@ async def test_prepare_cloned_repo_dir_merged_pr(
async with runner_handler._prepare_cloned_repo_dir(
"/tmp/test-repo-unique5", mock_pull_request, is_merged=True
) as result:
success, out, err = result
success, _, _ = result
assert success is True

@pytest.mark.asyncio
Expand All @@ -583,7 +583,7 @@ async def run_command_side_effect(*args, **kwargs):
new=AsyncMock(side_effect=run_command_side_effect),
):
async with runner_handler._prepare_cloned_repo_dir("/tmp/test-repo-x", mock_pull_request) as result:
success, out, err = result
success, out, _ = result
assert not success
assert out == "fail"

Expand All @@ -605,7 +605,7 @@ async def run_command_side_effect(*args, **kwargs):
new=AsyncMock(side_effect=run_command_side_effect),
):
async with runner_handler._prepare_cloned_repo_dir("/tmp/test-repo-x", mock_pull_request) as result:
success, out, err = result
success, out, _ = result
assert not success
assert out == "fail"

Expand All @@ -627,7 +627,7 @@ async def run_command_side_effect(*args, **kwargs):
new=AsyncMock(side_effect=run_command_side_effect),
):
async with runner_handler._prepare_cloned_repo_dir("/tmp/test-repo-x", mock_pull_request) as result:
success, out, err = result
success, out, _ = result
assert not success
assert out == "fail"

Expand All @@ -653,7 +653,7 @@ async def run_command_side_effect(*args, **kwargs):
new=AsyncMock(side_effect=run_command_side_effect),
):
async with runner_handler._prepare_cloned_repo_dir("/tmp/test-repo-x", mock_pull_request) as result:
success, out, err = result
success, out, _ = result
assert not success
assert out == "fail"

Expand Down
20 changes: 10 additions & 10 deletions webhook_server/tests/test_webhook.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ def test_process_github_webhook_success_no_existing_hooks(
"""Test successful webhook creation when no existing hooks exist."""
mock_get_repo_api.return_value = mock_repo

success, message, log_func = process_github_webhook(
success, message, _ = process_github_webhook(
repository_name="test-repo", data=sample_data, webhook_ip="http://example.com", apis_dict=apis_dict
)

Expand Down Expand Up @@ -70,7 +70,7 @@ def test_process_github_webhook_success_with_secret(
"""Test successful webhook creation with secret."""
mock_get_repo_api.return_value = mock_repo

success, message, log_func = process_github_webhook(
success, _, _ = process_github_webhook(
repository_name="test-repo",
data=sample_data,
webhook_ip="http://example.com",
Expand Down Expand Up @@ -100,7 +100,7 @@ def test_process_github_webhook_default_events(
mock_get_repo_api.return_value = mock_repo
data_without_events = {"name": "owner/test-repo"}

success, message, log_func = process_github_webhook(
success, _, _ = process_github_webhook(
repository_name="test-repo", data=data_without_events, webhook_ip="http://example.com", apis_dict=apis_dict
)

Expand Down Expand Up @@ -129,7 +129,7 @@ def test_process_github_webhook_existing_hook_same_config(
mock_repo.get_hooks.return_value = [existing_hook]
mock_get_repo_api.return_value = mock_repo

success, message, log_func = process_github_webhook(
success, message, _ = process_github_webhook(
repository_name="test-repo", data=sample_data, webhook_ip="http://example.com", apis_dict=apis_dict
)

Expand Down Expand Up @@ -160,7 +160,7 @@ def test_process_github_webhook_secret_mismatch_deletes_old_hook(
mock_repo.get_hooks.return_value = [existing_hook]
mock_get_repo_api.return_value = mock_repo

success, message, log_func = process_github_webhook(
success, _, _ = process_github_webhook(
repository_name="test-repo",
data=sample_data,
webhook_ip="http://example.com",
Expand Down Expand Up @@ -204,7 +204,7 @@ def test_process_github_webhook_secret_removal_deletes_old_hook(
mock_repo.get_hooks.return_value = [existing_hook]
mock_get_repo_api.return_value = mock_repo

success, message, log_func = process_github_webhook(
success, _, _ = process_github_webhook(
repository_name="test-repo",
data=sample_data,
webhook_ip="http://example.com",
Expand Down Expand Up @@ -234,7 +234,7 @@ def test_process_github_webhook_missing_api(self, sample_data: dict[str, Any]) -
}
}

success, message, log_func = process_github_webhook(
success, message, _ = process_github_webhook(
repository_name="test-repo", data=sample_data, webhook_ip="http://example.com", apis_dict=apis_dict_no_api
)

Expand All @@ -249,7 +249,7 @@ def test_process_github_webhook_repository_not_found(
"""Test error handling when repository cannot be found."""
mock_get_repo_api.return_value = None

success, message, log_func = process_github_webhook(
success, message, _ = process_github_webhook(
repository_name="test-repo", data=sample_data, webhook_ip="http://example.com", apis_dict=apis_dict
)

Expand All @@ -270,7 +270,7 @@ def test_process_github_webhook_hooks_listing_error(
mock_repo.get_hooks.side_effect = Exception("Permission denied")
mock_get_repo_api.return_value = mock_repo

success, message, log_func = process_github_webhook(
success, message, _ = process_github_webhook(
repository_name="test-repo", data=sample_data, webhook_ip="http://example.com", apis_dict=apis_dict
)

Expand Down Expand Up @@ -299,7 +299,7 @@ def test_process_github_webhook_multiple_existing_hooks(
mock_repo.get_hooks.return_value = [matching_hook, non_matching_hook]
mock_get_repo_api.return_value = mock_repo

success, message, log_func = process_github_webhook(
success, message, _ = process_github_webhook(
repository_name="test-repo", data=sample_data, webhook_ip="http://example.com", apis_dict=apis_dict
)

Expand Down
2 changes: 1 addition & 1 deletion webhook_server/utils/app_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,5 +115,5 @@ def parse_datetime_string(datetime_str: str | None, field_name: str) -> datetime
except ValueError as e:
raise HTTPException(
status_code=400,
detail=f"Invalid {field_name} format: {datetime_str}. Expected ISO 8601 format. Error: {str(e)}",
detail=f"Invalid {field_name} format: {datetime_str}. Expected ISO 8601 format. Error: {e!s}",
) from e
31 changes: 20 additions & 11 deletions webhook_server/utils/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -470,19 +470,28 @@ def log_rate_limit(rate_limit: RateLimitOverview, api_user: str) -> None:

def get_future_results(futures: list[Future]) -> None:
"""
result must return tuple[bool, str, Callable] when the Callable is Logger function (LOGGER.info, LOGGER.error, etc)
"""
for result in as_completed(futures):
_res = result.result()
_log = _res[2]
if result.exception():
_log(result.exception())
Process futures from repository configuration tasks.

if _res[0]:
_log(_res[1])
Args:
futures: List of futures that return (success, message, logger_func) tuples

else:
_log(_res[1])
Notes:
Continues processing on exceptions to handle partial failures gracefully.
Worker threads may crash on archived repositories or API permission issues.
"""
logger = get_logger_with_params()

for result in as_completed(futures):
try:
# CRITICAL FIX: Calling result.result() will raise exception if one exists
# This gives us proper exception context for logger.exception()
_, message, logger_func = result.result()
logger_func(message)
except Exception:
# Proper exception context - logger.exception() can capture traceback
logger.exception(
"Repository configuration crashed. Check for archived repositories or API permission issues."
)


def get_repository_color_for_log_prefix(repository_name: str, data_dir: str) -> str:
Expand Down