Skip to content
Merged
Changes from 1 commit
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
32 changes: 32 additions & 0 deletions pr_split/cli.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
from __future__ import annotations

import json as json_mod
import shutil
import tempfile
import time
import urllib.request
from concurrent.futures import ThreadPoolExecutor, as_completed
from pathlib import Path
from threading import Lock, Semaphore
Expand Down Expand Up @@ -666,6 +668,19 @@ def _poll_for_merged(
return actually_merged


def _send_webhook(url: str, payload: dict[str, object]) -> None:
try:
data = json_mod.dumps(payload).encode("utf-8")
req = urllib.request.Request(
url, data=data, headers={"Content-Type": "application/json"}
)
with urllib.request.urlopen(req, timeout=10) as resp:
resp.read()
logger.info(f"Webhook notification sent to {url}")
except Exception as exc:
logger.warning(f"Failed to send webhook notification: {exc}")
Comment on lines +680 to +681
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Catching a broad Exception is generally discouraged as it can mask unexpected errors and make debugging harder. It's better to catch more specific exceptions that you anticipate from the operations in the try block. For this block, OSError (which covers network errors like URLError and HTTPError), ValueError (for URL issues), and TypeError (for JSON serialization) would be more appropriate.

Suggested change
except Exception as exc:
logger.warning(f"Failed to send webhook notification: {exc}")
except (OSError, ValueError, TypeError) as exc:
logger.warning(f"Failed to send webhook notification: {exc}")



@app.command(
name="merge",
help="Merge all split PRs in dependency order. Skips already-merged PRs.",
Expand All @@ -674,6 +689,14 @@ def merge_all(
auto: Annotated[
bool, typer.Option("--auto", help="Queue merges to run after CI checks pass")
] = False,
notify: Annotated[
str | None,
typer.Option(
"--notify",
help="Webhook URL to POST merge results to",
envvar="PR_SPLIT_WEBHOOK_URL",
),
] = None,
) -> None:
if not plan_exists():
console.print(ErrorMsg.NO_PLAN())
Expand Down Expand Up @@ -784,6 +807,15 @@ def merge_all(
console.print(f"[yellow]Skipped ({len(skipped)}): {', '.join(skipped)}[/yellow]")
if failed:
console.print(f"[red]Failed ({len(failed)}): {', '.join(failed)}[/red]")
if notify:
_send_webhook(notify, {
"event": "merge_complete",
"merged": merged,
"skipped": skipped,
"failed": failed,
"success": not (failed or stopped or exited_early),
})
Comment on lines +810 to +827
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Sending the webhook is a blocking network call that can delay the command's completion by up to 10 seconds. To provide a better user experience, it's best to send this notification in a background thread. You can use the already imported ThreadPoolExecutor to achieve this in a non-blocking way.

Suggested change
if notify:
_send_webhook(notify, {
"event": "merge_complete",
"merged": merged,
"skipped": skipped,
"failed": failed,
"success": not (failed or stopped or exited_early),
})
if notify:
executor = ThreadPoolExecutor(max_workers=1)
executor.submit(_send_webhook, notify, {
"event": "merge_complete",
"merged": merged,
"skipped": skipped,
"failed": failed,
"success": not (failed or stopped or exited_early),
})
executor.shutdown(wait=False)


if failed or stopped or exited_early:
raise typer.Exit(1)
logger.success(f"Merge complete: {len(merged)} PRs merged")
Loading