diff --git a/src/oca_github_bot/manifest.py b/src/oca_github_bot/manifest.py index 59800510..871f8ce3 100644 --- a/src/oca_github_bot/manifest.py +++ b/src/oca_github_bot/manifest.py @@ -258,34 +258,95 @@ 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 + org, repo, username, modified_addons, other_branches, gh=gh ) -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 _get_manifest_from_raw(org, repo, addon, branch, manifest_file): + """Fallback to read an addon manifest from github raw URLs. + + Returns the parsed manifest dict, or None if the file does not exist or + cannot be read. + """ + url = f"https://github.com/{org}/{repo}/raw/{branch}/{addon}/{manifest_file}" + _logger.debug("Looking for maintainers in %s", url) + try: + r = requests.get( + url, allow_redirects=True, headers={"Cache-Control": "no-cache"} + ) + except requests.RequestException as e: + _logger.warning("Failed to fetch %s: %s", url, e) + return None + if not r.ok: + return None + try: + return parse_manifest(r.content) + except Exception as e: + _logger.warning("Failed to parse manifest from %s: %s", url, e) + return None + + +def is_maintainer_other_branches( + org, repo, username, modified_addons, other_branches, gh=None +): + """Check if username is maintainer of modified_addons in any configured branch. + + When an authenticated GitHub session is available, the GitHub contents API + is used instead of unauthenticated raw.githubusercontent.com requests. This + avoids rate-limiting and transient network errors that could spuriously deny + maintainer privileges during migrations. + """ + gh_repo = None + if gh is not None: + try: + gh_repo = gh.repository(org, repo) + except Exception as e: + _logger.warning("Could not get repository %s/%s from API: %s", org, repo, e) + 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 = None + if gh_repo is not None: + manifest = _get_manifest_from_api(gh_repo, addon, branch, manifest_file) + if manifest is None: + manifest = _get_manifest_from_raw( + org, 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..b1d52827 100644 --- a/tests/test_manifest.py +++ b/tests/test_manifest.py @@ -249,3 +249,40 @@ def test_is_maintainer_other_branches(): assert not is_maintainer_other_branches( "OCA", "mis-builder", "fpdoo", {"mis_builder"}, ["12.0"] ) + + +def test_is_maintainer_other_branches_with_gh(mocker): + """When an authenticated GitHub session is provided, the API is used. + + The raw URL fallback must not be hit if the API returns a valid manifest. + """ + gh_mock = mocker.MagicMock() + file_contents_mock = mocker.MagicMock() + file_contents_mock.content = b"{'name': 'addon1', 'maintainers': ['u1']}" + gh_repo_mock = mocker.MagicMock() + gh_repo_mock.file_contents.return_value = file_contents_mock + gh_mock.repository.return_value = gh_repo_mock + + assert is_maintainer_other_branches( + "OCA", "repo", "u1", {"addon1"}, ["15.0"], gh=gh_mock + ) + gh_repo_mock.file_contents.assert_called_once_with( + "addon1/__manifest__.py", ref="15.0" + ) + + +def test_is_maintainer_other_branches_api_fallback_to_raw(mocker): + """If the GitHub API returns no manifest, the raw URL fallback is used.""" + gh_mock = mocker.MagicMock() + gh_repo_mock = mocker.MagicMock() + gh_repo_mock.file_contents.return_value = None + gh_mock.repository.return_value = gh_repo_mock + raw_mock = mocker.patch( + "oca_github_bot.manifest._get_manifest_from_raw", + return_value={"name": "addon1", "maintainers": ["u1"]}, + ) + + assert is_maintainer_other_branches( + "OCA", "repo", "u1", {"addon1"}, ["15.0"], gh=gh_mock + ) + raw_mock.assert_called_once_with("OCA", "repo", "addon1", "15.0", "__manifest__.py")