From 98818a90ef78f1f703fc64701e4fcf15ebb4869f Mon Sep 17 00:00:00 2001 From: dbasunag Date: Thu, 27 Feb 2025 18:39:33 -0500 Subject: [PATCH 1/4] read branch protection rules from config files --- .../tests/test_branch_protection.py | 102 ++++++++++++++++++ .../utils/github_repository_settings.py | 47 ++++++-- 2 files changed, 143 insertions(+), 6 deletions(-) create mode 100644 webhook_server_container/tests/test_branch_protection.py diff --git a/webhook_server_container/tests/test_branch_protection.py b/webhook_server_container/tests/test_branch_protection.py new file mode 100644 index 00000000..30f372ca --- /dev/null +++ b/webhook_server_container/tests/test_branch_protection.py @@ -0,0 +1,102 @@ +import os +import pytest +from webhook_server_container.libs.config import Config +from webhook_server_container.utils.github_repository_settings import ( + get_repo_branch_protection_rules, + DEFAULT_BRANCH_PROTECTION, +) + + +@pytest.fixture() +def branch_protection_rules(request, mocker): + os.environ["WEBHOOK_SERVER_DATA_DIR"] = "webhook_server_container/tests/manifests" + config = Config() + repo_name = next(iter(config.data["repositories"])) + data = config.data + for key in request.param.get("global", {}): + data[key] = request.param["global"][key] + for key in request.param.get("repo", {}): + data["repositories"][repo_name][key] = request.param["repo"][key] + mocker.patch( + "webhook_server_container.libs.config.Config.data", new_callable=mocker.PropertyMock, return_value=data + ) + return get_repo_branch_protection_rules(config_data=config.data, repo_data=config.data["repositories"][repo_name]) + + +@pytest.mark.parametrize( + "branch_protection_rules, expected", + [ + pytest.param( + { + "global": { + "strict": True, + }, + "repo": { + "strict": False, + }, + }, + { + "strict": False, + }, + id="test_repo_branch_protection_rule", + ), + pytest.param( + {}, + { + "strict": True, + }, + id="test_default_branch_protection_rule", + ), + pytest.param( + { + "global": { + "strict": False, + }, + }, + { + "strict": False, + }, + id="test_global_branch_protection_rule", + ), + pytest.param( + { + "global": { + "strict": False, + "require_code_owner_reviews": True, + "dismiss_stale_reviews": False, + "required_approving_review_count": 2, + "required_linear_history": False, + }, + "repo": { + "strict": True, + "require_code_owner_reviews": True, + "dismiss_stale_reviews": False, + "required_approving_review_count": 1, + "required_linear_history": True, + }, + }, + { + "strict": True, + "require_code_owner_reviews": True, + "dismiss_stale_reviews": False, + "required_approving_review_count": 1, + "required_linear_history": True, + }, + id="test_repo_multiple_branch_protection_rule", + ), + pytest.param( + {}, + { + **DEFAULT_BRANCH_PROTECTION, + }, + id="test_default_branch_protection_rule", + ), + ], + indirect=["branch_protection_rules"], +) +def test_branch_protection_setup(branch_protection_rules, expected): + mismatch = {} + for key in expected: + if branch_protection_rules[key] != expected[key]: + mismatch[key] = f"Expected value for {key}: {expected[key]}, actual: {branch_protection_rules[key]}" + assert not mismatch, f"Following mismatches are found: {mismatch}" diff --git a/webhook_server_container/utils/github_repository_settings.py b/webhook_server_container/utils/github_repository_settings.py index 200071e2..05dc154e 100644 --- a/webhook_server_container/utils/github_repository_settings.py +++ b/webhook_server_container/utils/github_repository_settings.py @@ -30,6 +30,15 @@ get_logger_with_params, ) +DEFAULT_BRANCH_PROTECTION = { + "strict": True, + "require_code_owner_reviews": False, + "dismiss_stale_reviews": True, + "required_approving_review_count": 0, + "required_linear_history": True, + "required_conversation_resolution": True, +} + def _get_github_repo_api(github_api: github.Github, repository: int | str) -> Repository | None: logger = get_logger_with_params(name="github-repository-settings") @@ -49,19 +58,25 @@ def set_branch_protection( repository: Repository, required_status_checks: List[str], github_api: Github, + strict: bool, + require_code_owner_reviews: bool, + dismiss_stale_reviews: bool, + required_approving_review_count: int, + required_linear_history: bool, + required_conversation_resolution: bool, ) -> bool: logger = get_logger_with_params(name="github-repository-settings") api_user = github_api.get_user().login logger.info(f"Set branch {branch} setting for {repository.name}. enabled checks: {required_status_checks}") branch.edit_protection( - strict=True, - required_conversation_resolution=True, + strict=strict, + required_conversation_resolution=required_conversation_resolution, contexts=required_status_checks, - require_code_owner_reviews=False, - dismiss_stale_reviews=True, - required_approving_review_count=0, - required_linear_history=True, + require_code_owner_reviews=require_code_owner_reviews, + dismiss_stale_reviews=dismiss_stale_reviews, + required_approving_review_count=required_approving_review_count, + required_linear_history=required_linear_history, users_bypass_pull_request_allowances=[api_user], teams_bypass_pull_request_allowances=[api_user], apps_bypass_pull_request_allowances=[api_user], @@ -168,6 +183,19 @@ def set_repository_labels(repository: Repository) -> str: return f"{repository}: Setting repository labels is done" +def get_repo_branch_protection_rules(config_data: dict[str, Any], repo_data: dict[str, Any]) -> dict[str, Any]: + for key in DEFAULT_BRANCH_PROTECTION: + branch_protection_rule = None + if key in repo_data: + branch_protection_rule = repo_data[key] + elif key in config_data: + branch_protection_rule = config_data[key] + else: + branch_protection_rule = DEFAULT_BRANCH_PROTECTION[key] + repo_data[key] = branch_protection_rule + return repo_data + + def set_repositories_settings(config_: Config, github_api: Github) -> None: logger = get_logger_with_params(name="github-repository-settings") @@ -186,6 +214,7 @@ def set_repositories_settings(config_: Config, github_api: Github) -> None: futures = [] with ThreadPoolExecutor() as executor: for _, data in config_data["repositories"].items(): + data = get_repo_branch_protection_rules(config_data=config_data, repo_data=data) futures.append( executor.submit( set_repository, @@ -247,6 +276,12 @@ def set_repository( set_branch_protection, **{ "branch": branch, + "strict": data["strict"], + "require_code_owner_reviews": data["require_code_owner_reviews"], + "dismiss_stale_reviews": data["dismiss_stale_reviews"], + "required_approving_review_count": data["required_approving_review_count"], + "required_linear_history": data["required_linear_history"], + "required_conversation_resolution": data["required_conversation_resolution"], "repository": repo, "required_status_checks": required_status_checks, "github_api": github_api, From eee17a584071d6d27b3fbc2e241bc019dc976a59 Mon Sep 17 00:00:00 2001 From: dbasunag Date: Fri, 28 Feb 2025 14:29:43 -0500 Subject: [PATCH 2/4] updates based on review comments --- webhook_server_container/config/schema.yaml | 24 +++++++++++++++++++ .../tests/test_branch_protection.py | 9 +------ .../utils/github_repository_settings.py | 17 +++++++------ 3 files changed, 33 insertions(+), 17 deletions(-) diff --git a/webhook_server_container/config/schema.yaml b/webhook_server_container/config/schema.yaml index b27e042f..d1097be7 100644 --- a/webhook_server_container/config/schema.yaml +++ b/webhook_server_container/config/schema.yaml @@ -51,6 +51,18 @@ properties: token: type: string format: password + strict: + type: boolean + require_code_owner_reviews: + type: boolean + dismiss_stale_reviews: + type: boolean + required_approving_review_count: + type: integer + required_linear_history: + type: boolean + required_conversation_resolution: + type: boolean repositories: type: object properties: @@ -141,6 +153,18 @@ properties: type: array items: type: string + strict: + type: boolean + require_code_owner_reviews: + type: boolean + dismiss_stale_reviews: + type: boolean + required_approving_review_count: + type: integer + required_linear_history: + type: boolean + required_conversation_resolution: + type: boolean can-be-merged-required-labels: type: array items: diff --git a/webhook_server_container/tests/test_branch_protection.py b/webhook_server_container/tests/test_branch_protection.py index 30f372ca..6df28998 100644 --- a/webhook_server_container/tests/test_branch_protection.py +++ b/webhook_server_container/tests/test_branch_protection.py @@ -11,7 +11,7 @@ def branch_protection_rules(request, mocker): os.environ["WEBHOOK_SERVER_DATA_DIR"] = "webhook_server_container/tests/manifests" config = Config() - repo_name = next(iter(config.data["repositories"])) + repo_name = "test-repo" data = config.data for key in request.param.get("global", {}): data[key] = request.param["global"][key] @@ -40,13 +40,6 @@ def branch_protection_rules(request, mocker): }, id="test_repo_branch_protection_rule", ), - pytest.param( - {}, - { - "strict": True, - }, - id="test_default_branch_protection_rule", - ), pytest.param( { "global": { diff --git a/webhook_server_container/utils/github_repository_settings.py b/webhook_server_container/utils/github_repository_settings.py index 05dc154e..36371309 100644 --- a/webhook_server_container/utils/github_repository_settings.py +++ b/webhook_server_container/utils/github_repository_settings.py @@ -28,6 +28,7 @@ get_api_with_highest_rate_limit, get_future_results, get_logger_with_params, + get_value_from_dicts, ) DEFAULT_BRANCH_PROTECTION = { @@ -184,15 +185,13 @@ def set_repository_labels(repository: Repository) -> str: def get_repo_branch_protection_rules(config_data: dict[str, Any], repo_data: dict[str, Any]) -> dict[str, Any]: - for key in DEFAULT_BRANCH_PROTECTION: - branch_protection_rule = None - if key in repo_data: - branch_protection_rule = repo_data[key] - elif key in config_data: - branch_protection_rule = config_data[key] - else: - branch_protection_rule = DEFAULT_BRANCH_PROTECTION[key] - repo_data[key] = branch_protection_rule + for rule_name in DEFAULT_BRANCH_PROTECTION: + repo_data[rule_name] = get_value_from_dicts( + primary_dict=repo_data, + secondary_dict=config_data, + key=rule_name, + return_on_none=DEFAULT_BRANCH_PROTECTION[rule_name], + ) return repo_data From 34097ac5deeb9f9bb4918d4a1610c3dbc751da1f Mon Sep 17 00:00:00 2001 From: dbasunag Date: Tue, 4 Mar 2025 12:33:19 -0500 Subject: [PATCH 3/4] use branch_protection dict --- example.config.yaml | 13 +++++ webhook_server_container/config/schema.yaml | 54 ++++++++++--------- .../tests/test_branch_protection.py | 10 ++-- .../utils/github_repository_settings.py | 23 ++++---- 4 files changed, 62 insertions(+), 38 deletions(-) diff --git a/example.config.yaml b/example.config.yaml index 0364ee56..dc9ba22b 100644 --- a/example.config.yaml +++ b/example.config.yaml @@ -30,6 +30,13 @@ jira: user-mapping: : # if github user is not the same as jira +branch_protection: + strict: True + require_code_owner_reviews: True + dismiss_stale_reviews: False + required_approving_review_count: 1 + required_linear_history: True + repositories: my-repository: name: my-org/my-repository @@ -94,3 +101,9 @@ repositories: : # if github user is not the same as jira conventional-title: "ci,docs,feat,fix,refactor,test,release" # Check PR title start with any of these words + : + branch_protection: + strict: True + require_code_owner_reviews: True + dismiss_stale_reviews: False + required_approving_review_count: 1 + required_linear_history: True diff --git a/webhook_server_container/config/schema.yaml b/webhook_server_container/config/schema.yaml index d1097be7..80373275 100644 --- a/webhook_server_container/config/schema.yaml +++ b/webhook_server_container/config/schema.yaml @@ -51,18 +51,21 @@ properties: token: type: string format: password - strict: - type: boolean - require_code_owner_reviews: - type: boolean - dismiss_stale_reviews: - type: boolean - required_approving_review_count: - type: integer - required_linear_history: - type: boolean - required_conversation_resolution: - type: boolean + branch_protection: + type: object + properties: + strict: + type: boolean + require_code_owner_reviews: + type: boolean + dismiss_stale_reviews: + type: boolean + required_approving_review_count: + type: integer + required_linear_history: + type: boolean + required_conversation_resolution: + type: boolean repositories: type: object properties: @@ -153,18 +156,21 @@ properties: type: array items: type: string - strict: - type: boolean - require_code_owner_reviews: - type: boolean - dismiss_stale_reviews: - type: boolean - required_approving_review_count: - type: integer - required_linear_history: - type: boolean - required_conversation_resolution: - type: boolean + branch_protection: + type: object + properties: + strict: + type: boolean + require_code_owner_reviews: + type: boolean + dismiss_stale_reviews: + type: boolean + required_approving_review_count: + type: integer + required_linear_history: + type: boolean + required_conversation_resolution: + type: boolean can-be-merged-required-labels: type: array items: diff --git a/webhook_server_container/tests/test_branch_protection.py b/webhook_server_container/tests/test_branch_protection.py index 6df28998..71160038 100644 --- a/webhook_server_container/tests/test_branch_protection.py +++ b/webhook_server_container/tests/test_branch_protection.py @@ -13,14 +13,14 @@ def branch_protection_rules(request, mocker): config = Config() repo_name = "test-repo" data = config.data - for key in request.param.get("global", {}): - data[key] = request.param["global"][key] - for key in request.param.get("repo", {}): - data["repositories"][repo_name][key] = request.param["repo"][key] + data.setdefault("branch_protection", request.param.get("global", {})) + data["repositories"][repo_name].setdefault("branch_protection", request.param.get("repo", {})) mocker.patch( "webhook_server_container.libs.config.Config.data", new_callable=mocker.PropertyMock, return_value=data ) - return get_repo_branch_protection_rules(config_data=config.data, repo_data=config.data["repositories"][repo_name]) + return get_repo_branch_protection_rules(config_data=config.data, repo_data=config.data["repositories"][repo_name])[ + "branch_protection" + ] @pytest.mark.parametrize( diff --git a/webhook_server_container/utils/github_repository_settings.py b/webhook_server_container/utils/github_repository_settings.py index 36371309..bc15be01 100644 --- a/webhook_server_container/utils/github_repository_settings.py +++ b/webhook_server_container/utils/github_repository_settings.py @@ -186,9 +186,10 @@ def set_repository_labels(repository: Repository) -> str: def get_repo_branch_protection_rules(config_data: dict[str, Any], repo_data: dict[str, Any]) -> dict[str, Any]: for rule_name in DEFAULT_BRANCH_PROTECTION: - repo_data[rule_name] = get_value_from_dicts( - primary_dict=repo_data, - secondary_dict=config_data, + repo_data.setdefault("branch_protection", {}) + repo_data["branch_protection"][rule_name] = get_value_from_dicts( + primary_dict=repo_data["branch_protection"], + secondary_dict=config_data.get("branch_protection", {}), key=rule_name, return_on_none=DEFAULT_BRANCH_PROTECTION[rule_name], ) @@ -275,12 +276,16 @@ def set_repository( set_branch_protection, **{ "branch": branch, - "strict": data["strict"], - "require_code_owner_reviews": data["require_code_owner_reviews"], - "dismiss_stale_reviews": data["dismiss_stale_reviews"], - "required_approving_review_count": data["required_approving_review_count"], - "required_linear_history": data["required_linear_history"], - "required_conversation_resolution": data["required_conversation_resolution"], + "strict": data["branch_protection"]["strict"], + "require_code_owner_reviews": data["branch_protection"]["require_code_owner_reviews"], + "dismiss_stale_reviews": data["branch_protection"]["dismiss_stale_reviews"], + "required_approving_review_count": data["branch_protection"][ + "required_approving_review_count" + ], + "required_linear_history": data["branch_protection"]["required_linear_history"], + "required_conversation_resolution": data["branch_protection"][ + "required_conversation_resolution" + ], "repository": repo, "required_status_checks": required_status_checks, "github_api": github_api, From 71bf62d6e02d7e6718675503712a804cd7d2a9cf Mon Sep 17 00:00:00 2001 From: dbasunag Date: Wed, 5 Mar 2025 18:11:39 -0500 Subject: [PATCH 4/4] address review comments --- example.config.yaml | 2 ++ .../utils/github_repository_settings.py | 13 ++----------- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/example.config.yaml b/example.config.yaml index dc9ba22b..fe6e9c2a 100644 --- a/example.config.yaml +++ b/example.config.yaml @@ -36,6 +36,7 @@ branch_protection: dismiss_stale_reviews: False required_approving_review_count: 1 required_linear_history: True + required_conversation_resolution: True repositories: my-repository: @@ -107,3 +108,4 @@ repositories: dismiss_stale_reviews: False required_approving_review_count: 1 required_linear_history: True + required_conversation_resolution: True diff --git a/webhook_server_container/utils/github_repository_settings.py b/webhook_server_container/utils/github_repository_settings.py index bc15be01..d2391be8 100644 --- a/webhook_server_container/utils/github_repository_settings.py +++ b/webhook_server_container/utils/github_repository_settings.py @@ -237,6 +237,7 @@ def set_repository( repository: str = data["name"] logger.info(f"Processing repository {repository}") protected_branches: Dict[str, Any] = data.get("protected-branches", {}) + repo_branch_protection_rules: Dict[str, Any] = data["branch_protection"] repo = _get_github_repo_api(github_api=github_api, repository=repository) if not repo: return False, f"{repository}: Failed to get repository", logger.error @@ -270,26 +271,16 @@ def set_repository( default_status_checks=_default_status_checks, exclude_status_checks=exclude_status_checks, ) - futures.append( executor.submit( set_branch_protection, **{ "branch": branch, - "strict": data["branch_protection"]["strict"], - "require_code_owner_reviews": data["branch_protection"]["require_code_owner_reviews"], - "dismiss_stale_reviews": data["branch_protection"]["dismiss_stale_reviews"], - "required_approving_review_count": data["branch_protection"][ - "required_approving_review_count" - ], - "required_linear_history": data["branch_protection"]["required_linear_history"], - "required_conversation_resolution": data["branch_protection"][ - "required_conversation_resolution" - ], "repository": repo, "required_status_checks": required_status_checks, "github_api": github_api, }, + **repo_branch_protection_rules, ) )