From d4640cc7038e28055bdae32130de5415c42c6d13 Mon Sep 17 00:00:00 2001 From: Jairo Llopis Date: Thu, 30 Jul 2026 16:47:14 +0100 Subject: [PATCH] [FIX] oca_github_bot: use authenticated GitHub API to check maintainers in other branches When a maintainer tried to merge or rebase a migration PR, the bot looked for maintainers in other branches by downloading each addon manifest from raw.githubusercontent.com. That endpoint is unauthenticated and subject to rate limiting, transient errors and caching issues, which made the check fail intermittently (see #226). Since the bot already requires a GitHub token for all its operations, keep only the authenticated GitHub Contents API path and drop the raw URL fallback. This removes unnecessary code and avoids preserving the very failure mode this fix addresses. Fixes #226 Assisted-by: OpenCode + Kimi k2.7-code --- src/oca_github_bot/manifest.py | 53 +++++++++++++++++++++++----------- tests/test_manifest.py | 48 ++++++++++++++++++++++++++++-- 2 files changed, 81 insertions(+), 20 deletions(-) diff --git a/src/oca_github_bot/manifest.py b/src/oca_github_bot/manifest.py index 59800510..0430659c 100644 --- a/src/oca_github_bot/manifest.py +++ b/src/oca_github_bot/manifest.py @@ -6,8 +6,6 @@ import os import re -import requests - from . import config from .github import git_get_current_branch, github_user_can_push from .process import check_call, check_output @@ -258,34 +256,55 @@ def user_can_push(gh, org, repo, username, addons_dir, target_branch): if result: return True - other_branches = config.MAINTAINER_CHECK_ODOO_RELEASES + other_branches = list(config.MAINTAINER_CHECK_ODOO_RELEASES) if target_branch in other_branches: other_branches.remove(target_branch) return is_maintainer_other_branches( - org, repo, username, modified_addons, other_branches + gh_repo, username, modified_addons, other_branches ) -def is_maintainer_other_branches(org, repo, username, modified_addons, other_branches): +def _get_manifest_from_api(gh_repo, addon, branch, manifest_file): + """Read an addon manifest from a GitHub branch using the authenticated API. + + Returns the parsed manifest dict, or None if the file does not exist or + cannot be read. + """ + path = f"{addon}/{manifest_file}" + try: + file_contents = gh_repo.file_contents(path, ref=branch) + except Exception as e: + _logger.debug("Could not read %s@%s via GitHub API: %s", path, branch, e) + return None + if file_contents is None: + return None + try: + return parse_manifest(file_contents.content) + except Exception as e: + _logger.warning( + "Failed to parse manifest %s@%s from GitHub API: %s", path, branch, e + ) + return None + + +def is_maintainer_other_branches(gh_repo, username, modified_addons, other_branches): + """Check if username is maintainer of modified_addons in any configured branch. + + The authenticated GitHub contents API is used to read manifests. This + avoids rate-limiting and transient network errors that could spuriously deny + maintainer privileges during migrations. + """ for addon in modified_addons: is_maintainer = False for branch in other_branches: manifest_file = ( "__openerp__.py" if float(branch) < 10.0 else "__manifest__.py" ) - url = ( - f"https://github.com/{org}/{repo}/raw/{branch}/{addon}/{manifest_file}" - ) - _logger.debug("Looking for maintainers in %s", url) - r = requests.get( - url, allow_redirects=True, headers={"Cache-Control": "no-cache"} - ) - if r.ok: - manifest = parse_manifest(r.content) - if username in manifest.get("maintainers", []): - is_maintainer = True - break + manifest = _get_manifest_from_api(gh_repo, addon, branch, manifest_file) + if manifest and username in manifest.get("maintainers", []): + is_maintainer = True + break if not is_maintainer: return False diff --git a/tests/test_manifest.py b/tests/test_manifest.py index b160dcf9..5f8e9c80 100644 --- a/tests/test_manifest.py +++ b/tests/test_manifest.py @@ -1,6 +1,7 @@ # Copyright (c) ACSONE SA/NV 2018 # Distributed under the MIT License (http://opensource.org/licenses/MIT). +import logging import subprocess import pytest @@ -242,10 +243,51 @@ def test_is_maintainer(tmp_path): assert not is_maintainer("u1", [tmp_path / "not_an_addon"]) -def test_is_maintainer_other_branches(): +def test_is_maintainer_other_branches(mocker): + gh_repo_mock = mocker.MagicMock() + + def _file_contents(path, ref=None): + # Mimic the real OCA/mis-builder 12.0 manifest maintainers. + if ref == "12.0" and path == "mis_builder/__manifest__.py": + content = b"{'name': 'mis_builder', 'maintainers': ['sbidoul']}" + else: + content = b"{'name': 'other'}" + file_contents = mocker.MagicMock() + file_contents.content = content + return file_contents + + gh_repo_mock.file_contents.side_effect = _file_contents + assert is_maintainer_other_branches( - "OCA", "mis-builder", "sbidoul", {"mis_builder"}, ["12.0"] + gh_repo_mock, "sbidoul", {"mis_builder"}, ["12.0"] + ) + assert not is_maintainer_other_branches( + gh_repo_mock, "fpdoo", {"mis_builder"}, ["12.0"] + ) + + +def test_is_maintainer_other_branches_with_gh(mocker): + """The manifest is read through the authenticated GitHub contents API.""" + gh_repo_mock = mocker.MagicMock() + file_contents_mock = mocker.MagicMock() + file_contents_mock.content = b"{'name': 'addon1', 'maintainers': ['u1']}" + gh_repo_mock.file_contents.return_value = file_contents_mock + + assert is_maintainer_other_branches(gh_repo_mock, "u1", {"addon1"}, ["15.0"]) + gh_repo_mock.file_contents.assert_called_once_with( + "addon1/__manifest__.py", ref="15.0" ) + + +def test_is_maintainer_other_branches_api_errors(mocker, caplog): + """A missing or unparseable manifest is ignored, not raised.""" + gh_repo_mock = mocker.MagicMock() + file_contents_mock = mocker.MagicMock() + file_contents_mock.content = b"garbage{" + gh_repo_mock.file_contents.side_effect = [None, file_contents_mock] + caplog.set_level(logging.WARNING, logger="oca_github_bot.manifest") + assert not is_maintainer_other_branches( - "OCA", "mis-builder", "fpdoo", {"mis_builder"}, ["12.0"] + gh_repo_mock, "u1", {"addon1"}, ["15.0", "14.0"] ) + assert "Failed to parse manifest addon1/__manifest__.py@14.0" in caplog.text