AbsaOSS / AbsaOSS/generate-release-notes

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

Abierto
#338 0 comentarios 0 reacciones 0 asignados Ver en GitHub
Lenguaje dominante
Python
Estrellas
13
Forks
0
Merge medio
6 h 50 min
PR fusionados (30 d)
7

Descripción

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)

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.