diff --git a/src/oca_github_bot/manifest.py b/src/oca_github_bot/manifest.py index 0430659c..fea3be5d 100644 --- a/src/oca_github_bot/manifest.py +++ b/src/oca_github_bot/manifest.py @@ -97,18 +97,6 @@ def set_manifest_version(addon_dir, version): f.write(manifest) -def is_maintainer(username, addon_dirs): - for addon_dir in addon_dirs: - try: - manifest = get_manifest(addon_dir) - except NoManifestFound: - return False - maintainers = manifest.get("maintainers", []) - if username not in maintainers: - return False - return True - - def bump_version(version, mode): mo = VERSION_RE.match(version) if not mo: @@ -249,20 +237,14 @@ def user_can_push(gh, org, repo, username, addons_dir, target_branch): current_branch = git_get_current_branch(cwd=addons_dir) try: check_call(["git", "checkout", target_branch], cwd=addons_dir) - result = is_maintainer(username, modified_addon_dirs) + result = is_maintainer(gh_repo, username, modified_addon_dirs) finally: check_call(["git", "checkout", current_branch], cwd=addons_dir) if result: return True - other_branches = list(config.MAINTAINER_CHECK_ODOO_RELEASES) - if target_branch in other_branches: - other_branches.remove(target_branch) - - return is_maintainer_other_branches( - gh_repo, username, modified_addons, other_branches - ) + return is_maintainer(gh_repo, username, modified_addons) def _get_manifest_from_api(gh_repo, addon, branch, manifest_file): @@ -288,24 +270,49 @@ def _get_manifest_from_api(gh_repo, addon, branch, manifest_file): 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. +def get_maintainers(gh_repo, modified_addons, branches=None): + """Get maintainers of modified_addons in `branches`. 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. """ + if not branches: + branches = config.MAINTAINER_CHECK_ODOO_RELEASES + + maintainers_dict = dict() 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" - ) + maintainers_dict[addon] = set() + for branch in branches: + try: + branch_version = float(branch) + except ValueError: + # master, main, ... + manifest_file = "__manifest__.py" + else: + manifest_file = ( + "__openerp__.py" if branch_version < 10.0 else "__manifest__.py" + ) manifest = _get_manifest_from_api(gh_repo, addon, branch, manifest_file) - if manifest and username in manifest.get("maintainers", []): - is_maintainer = True - break + if manifest: + branch_maintainers = manifest.get("maintainers", set()) + maintainers_dict[addon] = maintainers_dict[addon].union( + branch_maintainers + ) + return maintainers_dict + + +def is_maintainer(gh_repo, username, modified_addons, branches=None): + """Check if username is maintainer of modified_addons in any configured branch. - if not is_maintainer: + 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. + """ + if not branches: + branches = config.MAINTAINER_CHECK_ODOO_RELEASES + maintainers_dict = get_maintainers(gh_repo, modified_addons, branches=branches) + for _addon, maintainers in maintainers_dict.items(): + if username not in maintainers: return False return True diff --git a/src/oca_github_bot/tasks/mention_maintainer.py b/src/oca_github_bot/tasks/mention_maintainer.py index 4b4649e1..e166052b 100644 --- a/src/oca_github_bot/tasks/mention_maintainer.py +++ b/src/oca_github_bot/tasks/mention_maintainer.py @@ -4,8 +4,7 @@ from .. import config, github from ..config import switchable from ..manifest import ( - addon_dirs_in, - get_manifest, + get_maintainers, git_modified_addon_dirs, is_addon_dir, ) @@ -22,10 +21,6 @@ def mention_maintainer(org, repo, pr, dry_run=False): gh_pr = gh.pull_request(org, repo, pr) target_branch = gh_pr.base.ref with github.temporary_clone(org, repo, target_branch) as clonedir: - # Get maintainers existing before the PR changes - addon_dirs = addon_dirs_in(clonedir, installable_only=True) - maintainers_dict = get_maintainers(addon_dirs) - # Get list of addons modified in the PR. pr_branch = f"tmp-pr-{pr}" check_call( @@ -41,6 +36,10 @@ def mention_maintainer(org, repo, pr, dry_run=False): d for d in modified_addon_dirs if is_addon_dir(d, installable_only=True) ] + # Get maintainers existing before the PR changes + gh_repo = gh.repository(org, repo) + maintainers_dict = get_maintainers(gh_repo, modified_addon_dirs) + modified_addons_maintainers = set() for modified_addon in modified_addon_dirs: addon_maintainers = maintainers_dict.get(modified_addon, list()) @@ -81,15 +80,3 @@ def get_adopt_mention(pr_opener): if config.ADOPT_AN_ADDON_MENTION: return config.ADOPT_AN_ADDON_MENTION.format(pr_opener=pr_opener) return None - - -def get_maintainers(addon_dirs): - """Get maintainer for each addon in `addon_dirs`. - - :return: Dictionary {'addon_dir': } - """ - addon_maintainers_dict = dict() - for addon_dir in addon_dirs: - maintainers = get_manifest(addon_dir).get("maintainers", []) - addon_maintainers_dict.setdefault(addon_dir, maintainers) - return addon_maintainers_dict diff --git a/tests/test_manifest.py b/tests/test_manifest.py index 5f8e9c80..af6f0c78 100644 --- a/tests/test_manifest.py +++ b/tests/test_manifest.py @@ -21,10 +21,11 @@ is_addon_dir, is_addons_dir, is_maintainer, - is_maintainer_other_branches, set_manifest_version, ) +from .common import set_config + def test_is_addons_dir_empty(tmpdir): tmpdir.mkdir("addon") @@ -223,7 +224,7 @@ def test_get_odoo_series_from_version(): get_odoo_series_from_version("12.0.1") -def test_is_maintainer(tmp_path): +def test_is_maintainer(mocker, tmp_path): addon1 = tmp_path / "addon1" addon1.mkdir() (addon1 / "__manifest__.py").write_text( @@ -235,12 +236,22 @@ def test_is_maintainer(tmp_path): addon3 = tmp_path / "addon3" addon3.mkdir() (addon3 / "__manifest__.py").write_text("{'name': 'addon3'}") - assert is_maintainer("u1", [addon1]) - assert not is_maintainer("u1", [addon2]) - assert not is_maintainer("u1", [addon1, addon2]) - assert is_maintainer("u2", [addon1, addon2]) - assert not is_maintainer("u2", [addon1, addon2, addon3]) - assert not is_maintainer("u1", [tmp_path / "not_an_addon"]) + gh_repo_mock = mocker.MagicMock() + + def _file_contents(path, ref=None): + file_contents = mocker.MagicMock() + file_contents.content = (tmp_path / path).read_bytes() + return file_contents + + gh_repo_mock.file_contents.side_effect = _file_contents + + with set_config(MAINTAINER_CHECK_ODOO_RELEASES="10.0"): + assert is_maintainer(gh_repo_mock, "u1", [addon1]) + assert not is_maintainer(gh_repo_mock, "u1", [addon2]) + assert not is_maintainer(gh_repo_mock, "u1", [addon1, addon2]) + assert is_maintainer(gh_repo_mock, "u2", [addon1, addon2]) + assert not is_maintainer(gh_repo_mock, "u2", [addon1, addon2, addon3]) + assert not is_maintainer(gh_repo_mock, "u1", [tmp_path / "not_an_addon"]) def test_is_maintainer_other_branches(mocker): @@ -258,12 +269,8 @@ def _file_contents(path, ref=None): gh_repo_mock.file_contents.side_effect = _file_contents - assert is_maintainer_other_branches( - gh_repo_mock, "sbidoul", {"mis_builder"}, ["12.0"] - ) - assert not is_maintainer_other_branches( - gh_repo_mock, "fpdoo", {"mis_builder"}, ["12.0"] - ) + assert is_maintainer(gh_repo_mock, "sbidoul", {"mis_builder"}, ["12.0"]) + assert not is_maintainer(gh_repo_mock, "fpdoo", {"mis_builder"}, ["12.0"]) def test_is_maintainer_other_branches_with_gh(mocker): @@ -273,7 +280,7 @@ def test_is_maintainer_other_branches_with_gh(mocker): 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"]) + assert is_maintainer(gh_repo_mock, "u1", {"addon1"}, ["15.0"]) gh_repo_mock.file_contents.assert_called_once_with( "addon1/__manifest__.py", ref="15.0" ) @@ -287,7 +294,5 @@ def test_is_maintainer_other_branches_api_errors(mocker, caplog): 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( - gh_repo_mock, "u1", {"addon1"}, ["15.0", "14.0"] - ) + assert not is_maintainer(gh_repo_mock, "u1", {"addon1"}, ["15.0", "14.0"]) assert "Failed to parse manifest addon1/__manifest__.py@14.0" in caplog.text diff --git a/tests/test_mention_maintainer.py b/tests/test_mention_maintainer.py index ddf93661..7f14436e 100644 --- a/tests/test_mention_maintainer.py +++ b/tests/test_mention_maintainer.py @@ -1,9 +1,12 @@ # Copyright 2019 Simone Rubino - Agile Business Group # Distributed under the MIT License (http://opensource.org/licenses/MIT). -import shutil + +import json +import subprocess import pytest +from oca_github_bot.github import git_get_current_branch from oca_github_bot.tasks.mention_maintainer import mention_maintainer from .common import make_addon, set_config @@ -13,6 +16,18 @@ def test_maintainer_mentioned(git_clone, mocker): github_mock = mocker.patch("oca_github_bot.tasks.mention_maintainer.github") github_mock.temporary_clone.return_value.__enter__.return_value = str(git_clone) + pr_mock = github_mock.login.return_value.__enter__.return_value.pull_request + pr_mock.return_value.base.ref = git_get_current_branch(git_clone) + gh_repo_mock = mocker.MagicMock() + repo_mock = github_mock.login.return_value.__enter__.return_value.repository + repo_mock.return_value = gh_repo_mock + + def _file_contents(path, ref=None): + file_contents = mocker.MagicMock() + file_contents.content = (git_clone / path).read_bytes() + return file_contents + + gh_repo_mock.file_contents.side_effect = _file_contents addon_name = "addon1" addon_dir = make_addon(git_clone, addon_name, maintainers=["themaintainer"]) @@ -22,52 +37,29 @@ def test_maintainer_mentioned(git_clone, mocker): ) modified_addons_mock.return_value = [addon_dir], False, {addon_name} mocker.patch("oca_github_bot.tasks.mention_maintainer.check_call") - mention_maintainer("org", "repo", "pr") + with set_config(MAINTAINER_CHECK_ODOO_RELEASES=["10.0"]): + mention_maintainer("org", "repo", "pr") github_mock.gh_call.assert_called_once() assert "@themaintainer" in github_mock.gh_call.mock_calls[0][1][1] @pytest.mark.vcr() -def test_added_maintainer_not_mentioned(git_clone, mocker): - """Only maintainers existing before the PR will be mentioned.""" +def test_multi_maintainer_one_mention(git_clone, mocker): github_mock = mocker.patch("oca_github_bot.tasks.mention_maintainer.github") github_mock.temporary_clone.return_value.__enter__.return_value = str(git_clone) + pr_mock = github_mock.login.return_value.__enter__.return_value.pull_request + pr_mock.return_value.base.ref = git_get_current_branch(git_clone) + gh_repo_mock = mocker.MagicMock() + repo_mock = github_mock.login.return_value.__enter__.return_value.repository + repo_mock.return_value = gh_repo_mock - addon_name = "addon1" - pre_pr_addon = make_addon(git_clone, addon_name, maintainers=["themaintainer"]) - pre_pr_addon_mock = mocker.patch( - "oca_github_bot.tasks.mention_maintainer.addon_dirs_in" - ) - - def pr_edited_addon(_args, **_kwargs): - shutil.rmtree(pre_pr_addon) - edited_addon = make_addon( - git_clone, addon_name, maintainers=["themaintainer", "added_maintainer"] - ) - return [str(edited_addon)] - - pre_pr_addon_mock.side_effect = pr_edited_addon - pre_pr_addon_mock.return_value = [pre_pr_addon], False - - modified_addons_mock = mocker.patch( - "oca_github_bot.tasks.mention_maintainer.git_modified_addon_dirs" - ) - modified_addons_mock.return_value = [pre_pr_addon], False, {addon_name} - - mocker.patch("oca_github_bot.tasks.mention_maintainer.check_call") - - mention_maintainer("org", "repo", "pr") - - github_mock.gh_call.assert_called_once() - assert "@themaintainer" in github_mock.gh_call.mock_calls[0][1][1] - assert "@added_maintainer" in github_mock.gh_call.mock_calls[0][1][1] - + def _file_contents(path, ref=None): + file_contents = mocker.MagicMock() + file_contents.content = (git_clone / path).read_bytes() + return file_contents -@pytest.mark.vcr() -def test_multi_maintainer_one_mention(git_clone, mocker): - github_mock = mocker.patch("oca_github_bot.tasks.mention_maintainer.github") - github_mock.temporary_clone.return_value.__enter__.return_value = str(git_clone) + gh_repo_mock.file_contents.side_effect = _file_contents addon_dirs = list() addon_names = ["addon1", "addon2"] @@ -81,7 +73,8 @@ def test_multi_maintainer_one_mention(git_clone, mocker): ) modified_addons_mock.return_value = addon_dirs, False, set(addon_names) mocker.patch("oca_github_bot.tasks.mention_maintainer.check_call") - mention_maintainer("org", "repo", "pr") + with set_config(MAINTAINER_CHECK_ODOO_RELEASES=["10.0"]): + mention_maintainer("org", "repo", "pr") github_mock.gh_call.assert_called_once() comment = github_mock.gh_call.mock_calls[0][1][1] @@ -95,6 +88,17 @@ def test_pr_by_maintainer_no_mention(git_clone, mocker): github_mock.temporary_clone.return_value.__enter__.return_value = str(git_clone) pr_mock = github_mock.login.return_value.__enter__.return_value.pull_request pr_mock.return_value.user.login = themaintainer + pr_mock.return_value.base.ref = git_get_current_branch(git_clone) + gh_repo_mock = mocker.MagicMock() + repo_mock = github_mock.login.return_value.__enter__.return_value.repository + repo_mock.return_value = gh_repo_mock + + def _file_contents(path, ref=None): + file_contents = mocker.MagicMock() + file_contents.content = (git_clone / path).read_bytes() + return file_contents + + gh_repo_mock.file_contents.side_effect = _file_contents addon_dirs = list() addon_names = ["addon1", "addon2"] @@ -107,7 +111,8 @@ def test_pr_by_maintainer_no_mention(git_clone, mocker): ) modified_addons_mock.return_value = addon_dirs, False, set(addon_names) mocker.patch("oca_github_bot.tasks.mention_maintainer.check_call") - mention_maintainer("org", "repo", "pr") + with set_config(MAINTAINER_CHECK_ODOO_RELEASES=["10.0"]): + mention_maintainer("org", "repo", "pr") github_mock.gh_call.assert_not_called() @@ -116,6 +121,8 @@ def test_pr_by_maintainer_no_mention(git_clone, mocker): def test_no_maintainer_adopt_module(git_clone, mocker): github_mock = mocker.patch("oca_github_bot.tasks.mention_maintainer.github") github_mock.temporary_clone.return_value.__enter__.return_value = str(git_clone) + pr_mock = github_mock.login.return_value.__enter__.return_value.pull_request + pr_mock.return_value.base.ref = git_get_current_branch(git_clone) addon_name = "addon1" addon_dir = make_addon(git_clone, addon_name) @@ -130,3 +137,67 @@ def test_no_maintainer_adopt_module(git_clone, mocker): mention_maintainer("org", "repo", "pr") github_mock.gh_call.assert_called_once() + + +@pytest.mark.vcr() +def test_mention_maintainer_other_branches(git_clone, mocker): + addon_name = "addon_maintainer_other_branches" + + # create `addon_name` on `other_branch` with maintainer `maintainer` + maintainer = "maintainer" + maintainer_branch = "other_branch_maintainer" + subprocess.check_call(["git", "checkout", "-b", maintainer_branch], cwd=git_clone) + addon1_dir = git_clone / addon_name + addon1_dir.mkdir(exist_ok=True) + manifest = { + "name": addon_name, + "maintainers": [ + maintainer, + ], + } + (addon1_dir / "__manifest__.py").write_text(json.dumps(manifest)) + subprocess.check_call(["git", "add", addon_name], cwd=git_clone) + subprocess.check_call( + ["git", "commit", "-m", "[BOT] Add with maintainer"], cwd=git_clone + ) + + # create `addon_name` on current branch with no maintainers + branch = "current_branch_no_maintainer" + subprocess.check_call(["git", "checkout", "-b", branch], cwd=git_clone) + addon1_dir = git_clone / addon_name + addon1_dir.mkdir(exist_ok=True) + manifest = { + "name": addon_name, + } + (addon1_dir / "__manifest__.py").write_text(json.dumps(manifest)) + subprocess.check_call(["git", "add", addon_name], cwd=git_clone) + subprocess.check_call( + ["git", "commit", "-m", "[BOT] Add without maintainer"], cwd=git_clone + ) + + github_mock = mocker.patch("oca_github_bot.tasks.mention_maintainer.github") + github_mock.temporary_clone.return_value.__enter__.return_value = str(git_clone) + pr_mock = github_mock.login.return_value.__enter__.return_value.pull_request + pr_mock.return_value.base.ref = git_get_current_branch(git_clone) + gh_repo_mock = mocker.MagicMock() + repo_mock = github_mock.login.return_value.__enter__.return_value.repository + repo_mock.return_value = gh_repo_mock + + def _file_contents(path, ref=None): + subprocess.check_call(["git", "checkout", ref], cwd=git_clone) + file_contents = mocker.MagicMock() + file_contents.content = (git_clone / path).read_bytes() + return file_contents + + gh_repo_mock.file_contents.side_effect = _file_contents + + modified_addons_mock = mocker.patch( + "oca_github_bot.tasks.mention_maintainer.git_modified_addon_dirs" + ) + modified_addons_mock.return_value = [git_clone / addon_name], False, {addon_name} + mocker.patch("oca_github_bot.tasks.mention_maintainer.check_call") + + with set_config(MAINTAINER_CHECK_ODOO_RELEASES=[maintainer_branch]): + mention_maintainer("org", "repo", "pr") + + assert "@" + maintainer in github_mock.gh_call.mock_calls[0][1][1] diff --git a/tests/test_merge_bot.py b/tests/test_merge_bot.py index 23183cdc..e709d132 100644 --- a/tests/test_merge_bot.py +++ b/tests/test_merge_bot.py @@ -7,7 +7,7 @@ from oca_github_bot.manifest import user_can_push -from .common import commit_addon, make_addon +from .common import commit_addon, make_addon, set_config @pytest.mark.vcr() @@ -16,13 +16,26 @@ def test_user_can_merge_team_member(git_clone, gh): @pytest.mark.vcr() -def test_user_can_merge_maintainer(git_clone, gh): +def test_user_can_merge_maintainer(git_clone, gh, mocker): make_addon(git_clone, "addon1", maintainers=["themaintainer"]) commit_addon(git_clone, "addon1") subprocess.check_call(["git", "checkout", "-b", "thebranch"], cwd=git_clone) (git_clone / "addon1" / "data").write_text("") commit_addon(git_clone, "addon1") - assert user_can_push(gh, "OCA", "mis-builder", "themaintainer", git_clone, "master") + gh_repo_mock = mocker.MagicMock() + gh.repository = lambda org, repo: gh_repo_mock + + def _file_contents(path, ref=None): + file_contents = mocker.MagicMock() + file_contents.content = (git_clone / path).read_bytes() + return file_contents + + gh_repo_mock.file_contents.side_effect = _file_contents + + with set_config(MAINTAINER_CHECK_ODOO_RELEASES=["master"]): + assert user_can_push( + gh, "OCA", "mis-builder", "themaintainer", git_clone, "master" + ) @pytest.mark.vcr()