AbsaOSS / AbsaOSS/generate-release-notes

Tech-debt: Since-time mode: `since=None` can crash `get_issues()`, and API failures silently degrade to empty results

Đang mở
#338 0 bình luận 0 reaction 0 người được giao Xem trên GitHub
Ngôn ngữ chính
Python
Star
13
Fork
0
Merge trung bình
6 giờ 50 phút
Pull request đã merge (30 ngày)
7

Mô tả

Found while reviewing PR #337 (Copilot comments on the pagination-safety refactor, since reverted from that PR as out of scope for #335).

## Problem 1 — `since=None` can raise inside PyGithub

In `DataMiner._get_issues`, `data.since` is `Optional[datetime]` and gets passed straight through to `Repository.get_issues(..., since=data.since)`. PyGithub's implementation asserts `is_optional(since, datetime)`, i.e. `isinstance(v, NotSetType) or isinstance(v, datetime)` — explicitly passing `None` satisfies neither, so `get_issues(since=None)` raises `AssertionError: None` instead of falling back to its default. Today this is masked because the call goes through `_safe_call`'s untyped wrapper, and is *also* practically unreachable in this exact spot (a GitHub release always has `created_at`) — but it's a latent crash risk, and the same pattern (attribute typed `Optional[datetime]`, passed unconditionally as a kwarg) could easily recur elsewhere.

Fix: only pass `since` when it's not `None` (omit the kwarg so PyGithub's `NotSet` default applies), instead of suppressing the type checker.

## Problem 2 — `_safe_call` failures silently produce incomplete results in since-time mode

`_safe_call` catches `GithubException`/network errors and returns `None`. In compare mode, a `None` result is treated as fatal (`sys.exit(1)`) — e.g. `comparison is None`. In since-time mode, several fetches (`repo.get_pulls`, `repo.get_commits`, `repo.get_releases`, `repo.get_issues`) instead do `... or []`, so a transient API failure silently becomes "0 PRs found" / "0 commits found" / "treat this as the first release" and the run completes "successfully" with incomplete or wrong release notes.

Decide and apply consistently: should a `_safe_call` failure during since-time mining be fatal (matching compare mode), or is partial/degraded output acceptable with a loud warning? Whichever is chosen, apply it uniformly across all the since-time-mode fetches instead of the current ad hoc mix of "crash" (pre-existing, for some paths) and "silently continue" (introduced in #337 for others).

Update - see latest [post from copilot review](https://github.com/AbsaOSS/generate-release-notes/pull/337#pullrequestreview-5007123333)

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.