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