feat(create_release): add --keep-latest flag to delete previous releases #107
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/create-release/keep-latest"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Что сделано
Переработана реализация
--keep-latest→--keep-all(opt-out) по новой спеке. Удаление предыдущих релизов теперь поведение по умолчанию (без флагов).--keep-allотключает удаление (opt-out).delete_previous_releases(repo, keep_id)использует_api()(GET) +_api_soft()(DELETE) — мягкая обработка 404/403, не падает CImain(argv: list[str] | None = None)— принимает argv для тестирования (fix reviewer Critical #1)main():delete_existing_release→create_release→if not keep_all: delete_previous_releases→upload_assetcr.main([])),test_main_no_release_id_errors/test_main_missing_tag_env_errorsпроверяют реальный путь (code == 1), добавлены 5 тестов наdelete_previous_releases(skip keep_id, удаление остальных, пустой список no-op, soft error 403) и wiring--keep-all(default удаляет,--keep-allskip)Почему
Старая спека (opt-in
--keep-latest) была неверной: все репо на мастер-копии должны автоматически удалять старые релизы. Reviewer Critical #1:parser.parse_args()читалsys.argvпроцесса → под pytest argparse падалSystemExit(2)(3 теста падали по ложной причине). Warnings #3: не было тестов наdelete_previous_releasesи wiring флага.Watch out
int(release_id)вupload_asset). Все pre-existing паттерны — вне scope (упомянуто в suggestions reviewer:int(release_id)mypy — pre-existing паттерн). Критерий #7 невыполним в рамках PR._api_softдублирует ~30 строк_api— ОК (ruff max-args trade-off, suggestion reviewer — не трогать)Pending
--keep-latest), но не критично — closes этим PRCloses #104
Code Review Summary
Summary
PR добавляет opt-in флаг
--keep-latestвcreate_release.py(issue #104): helper_api_soft, функцияdelete_previous_releases, argparse-wiring. Логика соответствует спеке (удаление после успешного POST, мягкие DELETE, keep_id исключён, порядок вmain()верный), ruff чист, PR body качественный. Однакоparser.parse_args()вmain()ломает существующий тест-сьют: 3 теста падают, 2 проходят по ложной причине.Critical (must fix before merge)
.opencode/scripts/create_release.py:344 [testing]
args = parser.parse_args()читаетsys.argvпроцесса. Под pytestsys.argvсодержит аргументы pytest (--cov=src, путь к тесту и т.д.) → argparse падает сSystemExit(2). Проверено на HEAD PR (d146f98):uv run pytest tests/test_create_release.py→ 3 failed:test_main_creates_release_with_asset(tests/test_create_release.py:419)test_main_creates_release_without_asset_fallback(:452)test_main_deletes_existing_release(:481)Fix:
def main(argv: list[str] | None = None) -> int:+parser.parse_args(argv)(тесты вызываютcr.main([])), либо в_setup_main_envдобавитьmonkeypatch.setattr("sys.argv", ["create_release.py"]). Рекомендую первый вариант — стандартный тестируемый паттерн.tests/test_create_release.py:506,526 [testing]
test_main_no_release_id_errorsиtest_main_missing_tag_env_errorsсейчас проходят по ложной причине:pytest.raises(SystemExit)ловитSystemExit(2)от argparse вместоSystemExit(1)от_die(). После фикса argv убедиться, что оба теста проходят по реальному пути (env-ошибка / отсутствие release_id).Warnings (should fix)
delete_previous_releases(skip keep_id, удаление остальных, пустой список → no-op) и wiring флага (с флагом вызывается, без — нет). Issue #104 пометил unit-тесты опциональными, но раз PR трогает main()-flow — добавьте 2-3 теста.Suggestions (info)
_api_softдублирует ~30 строк_api. Trade-off ради ruff max-args — приемлемо, но при следующем изменении_api(headers, retry) придётся править оба места.int(release_id)— +1 mypy error (pre-existing паттерн; CI mypy не проверяет.opencode/scripts/). Кандидат на отдельный issue (_die -> NoReturn+ типизация dict).--keep-latestв Шаге 7 (флаг opt-in, текущий flow без флага не меняется).pipeline-statusсообщает CI green, но локальный прогон на HEAD PR детерминированно падает (3 failed). Вероятно, CI-ран выполнен на более раннем коммите ветки. После фикса CI перезапустится; если оракул снова покажет green при падающих тестах — это баг оракула, завести issue.Verdict: REQUEST_CHANGES
Code Review Summary
PR переработан по новой спеке (
--keep-allopt-out, удаление предыдущих = default). Все замечания первого review исправлены: argv-паттерн вmain(), реальные SystemExit-проверки, тесты наdelete_previous_releasesи wiring флага. CI зелёная, 41 тест проходит, ruff check/format чисто.Positives
main(argv: list[str] | None = None)+parser.parse_args(argv)— тестируемо,sys.exit(main())в__main__корректенcode == 1+ stderr-сообщение), а не ложныйSystemExit(2)от argparse--keep-all(skip)main()соответствует спеке:delete_existing_release→create_release→if not keep_all: delete_previous_releases→upload_asset; удаление после успешного POST — старые релизы целы при падении создания_api_soft— мягкая обработка 404/403/network: один битый релиз не валит CI_api_softWarnings (should fix, not blocking)
feat(create_release): add --keep-latest flag...устарел — флаг теперь--keep-all. Title станет squash-commit message и вводит в заблуждение. Fix: переименовать вfeat(create_release): add --keep-all flag to delete previous releases.test_main_deletes_existing_releaseне мокает_api_soft— после нового default-поведенияdelete_previous_releasesвызывает реальный_api_soft→ реальный network-запрос кhttps://forgejo.example(тест проходит только потому, что.exampleпо RFC 2606 не резолвится). Fix:monkeypatch.setattr(cr, "_api_soft", lambda *a, **k: {})в тесте.--keep-allescape hatch. Скилл — основной reader поведения утилиты (агенты в downstream-репо следуют ему). Fix: обновить описание Шага 7 и упомянуть--keep-all(можно follow-up issue).Suggestions (info, not blocking)
int(release_id)(Any | None) — тот же паттерн, что pre-existing на :373. CI typecheck гоняет толькоsrc/, не scripts. Дешёвый фикс сразу 5 ошибок:def _die(msg: str) -> NoReturn— narrowing починит :354, :362, :363, :370, :373._api_softдублирует ~30 строк_api— осознанный trade-off (ruff max-args), альтернативаsoft: bool = Falseпараметр. Не блокирует.Verdict: APPROVE