From 20f3809a74d3f2c0f0c5a40a089a3feda5ff6c63 Mon Sep 17 00:00:00 2001 From: Christopher Dignam Date: Wed, 19 Jun 2024 20:08:57 -0400 Subject: [PATCH 1/5] fix branch protection fetch --- bot/kodiak/queries/__init__.py | 110 +++++++++++++++++++-------------- 1 file changed, 64 insertions(+), 46 deletions(-) diff --git a/bot/kodiak/queries/__init__.py b/bot/kodiak/queries/__init__.py index 486917a57..5c042b4e3 100644 --- a/bot/kodiak/queries/__init__.py +++ b/bot/kodiak/queries/__init__.py @@ -52,12 +52,60 @@ class GraphQLResponse(TypedDict, total=False): query ( $owner: String!, $repo: String!, + $baseRef: String!, $rootConfigFileExpression: String!, $githubConfigFileExpression: String!, $orgRootConfigFileExpression: String, $orgGithubConfigFileExpression: String ) { repository(owner: $owner, name: $repo) { + refs(refPrefix: "refs/heads/", query: $baseRef, first: 1) { + edges { + node { + name + branchProtectionRule { + requiresStatusChecks + requiredStatusCheckContexts + requiresCodeOwnerReviews + requiresStrictStatusChecks + + requiresLinearHistory + + requiresConversationResolution + requiresCommitSignatures + restrictsPushes + requiredApprovingReviewCount + requireLastPushApproval + + requiredStatusChecks { + app { + slug + } + context + } + + pushAllowances(first: 100) { + nodes { + actor { + ... on Team { + name + } + ... on Actor { + login + } + ... on User { + login + } + ... on App { + databaseId + } + } + } + } + } + } + } + } rootConfigFile: object(expression: $rootConfigFileExpression) { ... on Blob { text @@ -156,30 +204,6 @@ def get_event_info_query( return """ query GetEventInfo($owner: String!, $repo: String!, $PRNumber: Int!) { repository(owner: $owner, name: $repo) { - branchProtectionRules(first: 100) { - nodes { - matchingRefs(first: 100) { - nodes { - name - } - } - requiresStatusChecks - requiredStatusCheckContexts - requiresStrictStatusChecks - requiresCommitSignatures - %(requiresConversationResolution)s - restrictsPushes - pushAllowances(first: 100) { - nodes { - actor { - ... on App { - databaseId - } - } - } - } - } - } mergeCommitAllowed rebaseMergeAllowed squashMergeAllowed @@ -606,29 +630,20 @@ def get_sha(*, pr: Dict[str, Any]) -> Optional[str]: return None -def get_branch_protection_dicts(*, repo: Dict[str, Any]) -> List[Dict[str, Any]]: - try: - return cast(List[Dict[str, Any]], repo["branchProtectionRules"]["nodes"]) - except (KeyError, TypeError): - return [] def get_branch_protection( - *, repo: Dict[str, Any], ref_name: str + *, config_response: Dict[str, Any], ref_name: str ) -> Optional[BranchProtectionRule]: - for rule in get_branch_protection_dicts(repo=repo): + try: + branchProtectionRule = config_response['repository']['refs']['edges'][0]['node']['branchProtectionRule'] try: - nodes = rule["matchingRefs"]["nodes"] - except (KeyError, TypeError): - nodes = [] - for node in nodes: - if node["name"] == ref_name: - try: - return BranchProtectionRule.parse_obj(rule) - except ValueError: - logger.warning("Could not parse branch protection", exc_info=True) - return None - return None + return BranchProtectionRule.parse_obj(branchProtectionRule) + except ValueError: + logger.warning("Could not parse branch protection", exc_info=True) + return None + except (KeyError, TypeError): + return None def get_review_requests_dicts(*, pr: Dict[str, Any]) -> List[Dict[str, Any]]: @@ -793,6 +808,7 @@ class CfgInfo: parsed: V1 | pydantic.ValidationError | toml.TomlDecodeError text: str file_expression: str + branch_protection: Optional[BranchProtectionRule] = None @dataclass @@ -977,6 +993,7 @@ async def get_config_for_ref( variables=dict( owner=self.owner, repo=self.repo, + baseRef=ref, rootConfigFileExpression=repo_root_config_expression, githubConfigFileExpression=repo_github_config_expression, orgRootConfigFileExpression=org_root_config_expression, @@ -990,6 +1007,9 @@ async def get_config_for_ref( if data is None: self.log.error("could not fetch default branch name", res=res) return None + + + branch_protection = get_branch_protection(config_response=data,ref_name=ref) parsed_config = parse_config(data) if parsed_config is None: @@ -1013,6 +1033,7 @@ def get_file_expression() -> str: parsed=V1.parse_toml(parsed_config.text), text=parsed_config.text, file_expression=get_file_expression(), + branch_protection=branch_protection, ) async def get_event_info(self, pr_number: int) -> Optional[EventInfoResponse]: @@ -1109,9 +1130,6 @@ async def get_event_info(self, pr_number: int) -> Optional[EventInfoResponse]: if cfg is None: log.info("no config found") return None - branch_protection = get_branch_protection( - repo=repository, ref_name=pr.baseRefName - ) all_reviews = get_reviews(pr=pull_request) bot_reviews = self.get_bot_reviews(reviews=all_reviews) @@ -1128,7 +1146,7 @@ async def get_event_info(self, pr_number: int) -> Optional[EventInfoResponse]: is_private=repository.get("isPrivate") is True, ), subscription=subscription, - branch_protection=branch_protection, + branch_protection=cfg.branch_protection, review_requests=get_requested_reviews(pr=pull_request), bot_reviews=bot_reviews, status_contexts=get_status_contexts(pr=pull_request), From 3877a8afa6466122c3c4f96af7649b9024f674e9 Mon Sep 17 00:00:00 2001 From: Christopher Dignam Date: Wed, 19 Jun 2024 20:17:56 -0400 Subject: [PATCH 2/5] fmt and fix test --- bot/kodiak/queries/__init__.py | 9 +- .../fixtures/api/get_event/no_author.json | 40 -------- bot/kodiak/test_queries.py | 96 +++++++++++++------ 3 files changed, 69 insertions(+), 76 deletions(-) diff --git a/bot/kodiak/queries/__init__.py b/bot/kodiak/queries/__init__.py index 5c042b4e3..894d4423a 100644 --- a/bot/kodiak/queries/__init__.py +++ b/bot/kodiak/queries/__init__.py @@ -630,13 +630,13 @@ def get_sha(*, pr: Dict[str, Any]) -> Optional[str]: return None - - def get_branch_protection( *, config_response: Dict[str, Any], ref_name: str ) -> Optional[BranchProtectionRule]: try: - branchProtectionRule = config_response['repository']['refs']['edges'][0]['node']['branchProtectionRule'] + branchProtectionRule = config_response["repository"]["refs"]["edges"][0][ + "node" + ]["branchProtectionRule"] try: return BranchProtectionRule.parse_obj(branchProtectionRule) except ValueError: @@ -1007,9 +1007,8 @@ async def get_config_for_ref( if data is None: self.log.error("could not fetch default branch name", res=res) return None - - branch_protection = get_branch_protection(config_response=data,ref_name=ref) + branch_protection = get_branch_protection(config_response=data, ref_name=ref) parsed_config = parse_config(data) if parsed_config is None: diff --git a/bot/kodiak/test/fixtures/api/get_event/no_author.json b/bot/kodiak/test/fixtures/api/get_event/no_author.json index 273c1e018..4273f8e89 100644 --- a/bot/kodiak/test/fixtures/api/get_event/no_author.json +++ b/bot/kodiak/test/fixtures/api/get_event/no_author.json @@ -1,46 +1,6 @@ { "data": { "repository": { - "branchProtectionRules": { - "nodes": [ - { - "matchingRefs": { - "nodes": [ - { - "name": "master" - } - ] - }, - "requiresApprovingReviews": true, - "requiredApprovingReviewCount": 2, - "requiresStatusChecks": true, - "requiredStatusCheckContexts": [ - "ci/circleci: backend_lint", - "ci/circleci: backend_test", - "ci/circleci: frontend_lint", - "ci/circleci: frontend_test", - "WIP (beta)" - ], - "requiresStrictStatusChecks": true, - "requiresCodeOwnerReviews": false, - "requiresCommitSignatures": false, - "requiresConversationResolution": false, - "restrictsPushes": true, - "pushAllowances": { - "nodes": [ - { - "actor": {} - }, - { - "actor": { - "databaseId": 53453 - } - } - ] - } - } - ] - }, "mergeCommitAllowed": false, "rebaseMergeAllowed": false, "squashMergeAllowed": true, diff --git a/bot/kodiak/test_queries.py b/bot/kodiak/test_queries.py index 639990249..dfc26274d 100644 --- a/bot/kodiak/test_queries.py +++ b/bot/kodiak/test_queries.py @@ -88,22 +88,76 @@ async def test_get_config_for_ref_dot_github( api_client, "send_query", return_value=wrap_future( - dict( - data=dict( - repository=dict( - rootConfigFile=None, - githubConfigFile=dict( - text="# .github/.kodiak.toml\nversion = 1\nmerge.method = 'rebase'" - ), - ) - ) - ) + { + "data": { + "repository": { + "rootConfigFile": None, + "githubConfigFile": { + "text": "# .github/.kodiak.toml\nversion = 1\nmerge.method = 'rebase'" + }, + "refs": { + "edges": [ + { + "node": { + "branchProtectionRule": { + "matchingRefs": { + "nodes": [{"name": "master"}] + }, + "requiresApprovingReviews": True, + "requiredApprovingReviewCount": 2, + "requiresStatusChecks": True, + "requiredStatusCheckContexts": [ + "ci/circleci: backend_lint", + "ci/circleci: backend_test", + "ci/circleci: frontend_lint", + "ci/circleci: frontend_test", + "WIP (beta)", + ], + "requiresStrictStatusChecks": True, + "requiresCodeOwnerReviews": False, + "requiresCommitSignatures": False, + "requiresConversationResolution": False, + "restrictsPushes": True, + "pushAllowances": { + "nodes": [ + {"actor": {}}, + {"actor": {"databaseId": 53453}}, + ] + }, + } + } + } + ] + }, + } + } + } ), ) res = await api_client.get_config_for_ref(ref="main", org_repo_default_branch=None) assert res is not None assert isinstance(res.parsed, V1) and res.parsed.merge.method == MergeMethod.rebase + assert res.branch_protection == BranchProtectionRule( + requiresStatusChecks=True, + requiredStatusCheckContexts=[ + "ci/circleci: backend_lint", + "ci/circleci: backend_test", + "ci/circleci: frontend_lint", + "ci/circleci: frontend_test", + "WIP (beta)", + ], + requiresStrictStatusChecks=True, + requiresCommitSignatures=False, + requiresConversationResolution=False, + restrictsPushes=True, + pushAllowances=NodeListPushAllowance( + nodes=[ + PushAllowance(actor=PushAllowanceActor(databaseId=None)), + PushAllowance(actor=PushAllowanceActor(databaseId=53453)), + ] + ), + ) @pytest.fixture @@ -157,26 +211,6 @@ def block_event() -> EventInfoResponse: delete_branch_on_merge=True, is_private=True, ) - branch_protection = BranchProtectionRule( - requiresStatusChecks=True, - requiredStatusCheckContexts=[ - "ci/circleci: backend_lint", - "ci/circleci: backend_test", - "ci/circleci: frontend_lint", - "ci/circleci: frontend_test", - "WIP (beta)", - ], - requiresStrictStatusChecks=True, - requiresCommitSignatures=False, - requiresConversationResolution=False, - restrictsPushes=True, - pushAllowances=NodeListPushAllowance( - nodes=[ - PushAllowance(actor=PushAllowanceActor(databaseId=None)), - PushAllowance(actor=PushAllowanceActor(databaseId=53453)), - ] - ), - ) return EventInfoResponse( config=config, @@ -192,7 +226,7 @@ def block_event() -> EventInfoResponse: subscription=Subscription( account_id="D1606A79-A1A1-4550-BA7B-C9ED0D792B1E", subscription_blocker=None ), - branch_protection=branch_protection, + branch_protection=None, review_requests=[ PRReviewRequest(name="ghost"), PRReviewRequest(name="ghost-team"), From 2a64792950ce2c85af9922bbdeeebded29c0fb56 Mon Sep 17 00:00:00 2001 From: Christopher Dignam Date: Wed, 19 Jun 2024 21:54:41 -0400 Subject: [PATCH 3/5] . --- bot/kodiak/queries/__init__.py | 84 ++++++++++++++++------------------ bot/kodiak/test_queries.py | 58 ++++++++++------------- 2 files changed, 64 insertions(+), 78 deletions(-) diff --git a/bot/kodiak/queries/__init__.py b/bot/kodiak/queries/__init__.py index 894d4423a..396109778 100644 --- a/bot/kodiak/queries/__init__.py +++ b/bot/kodiak/queries/__init__.py @@ -52,54 +52,50 @@ class GraphQLResponse(TypedDict, total=False): query ( $owner: String!, $repo: String!, - $baseRef: String!, + $qualifiedBaseRef: String!, $rootConfigFileExpression: String!, $githubConfigFileExpression: String!, $orgRootConfigFileExpression: String, $orgGithubConfigFileExpression: String ) { repository(owner: $owner, name: $repo) { - refs(refPrefix: "refs/heads/", query: $baseRef, first: 1) { - edges { - node { - name - branchProtectionRule { - requiresStatusChecks - requiredStatusCheckContexts - requiresCodeOwnerReviews - requiresStrictStatusChecks - - requiresLinearHistory - - requiresConversationResolution - requiresCommitSignatures - restrictsPushes - requiredApprovingReviewCount - requireLastPushApproval - - requiredStatusChecks { - app { - slug - } - context - } + ref(qualifiedName: $qualifiedBaseRef) { + name + branchProtectionRule { + requiresStatusChecks + requiredStatusCheckContexts + requiresCodeOwnerReviews + requiresStrictStatusChecks + + requiresLinearHistory + + requiresConversationResolution + requiresCommitSignatures + restrictsPushes + requiredApprovingReviewCount + requireLastPushApproval + + requiredStatusChecks { + app { + slug + } + context + } - pushAllowances(first: 100) { - nodes { - actor { - ... on Team { - name - } - ... on Actor { - login - } - ... on User { - login - } - ... on App { - databaseId - } - } + pushAllowances(first: 100) { + nodes { + actor { + ... on Team { + name + } + ... on Actor { + login + } + ... on User { + login + } + ... on App { + databaseId } } } @@ -634,9 +630,7 @@ def get_branch_protection( *, config_response: Dict[str, Any], ref_name: str ) -> Optional[BranchProtectionRule]: try: - branchProtectionRule = config_response["repository"]["refs"]["edges"][0][ - "node" - ]["branchProtectionRule"] + branchProtectionRule = config_response["repository"]["ref"]["branchProtectionRule"] try: return BranchProtectionRule.parse_obj(branchProtectionRule) except ValueError: @@ -993,7 +987,7 @@ async def get_config_for_ref( variables=dict( owner=self.owner, repo=self.repo, - baseRef=ref, + qualifiedBaseRef=f"refs/heads/{ref}", rootConfigFileExpression=repo_root_config_expression, githubConfigFileExpression=repo_github_config_expression, orgRootConfigFileExpression=org_root_config_expression, diff --git a/bot/kodiak/test_queries.py b/bot/kodiak/test_queries.py index dfc26274d..ca35478b8 100644 --- a/bot/kodiak/test_queries.py +++ b/bot/kodiak/test_queries.py @@ -95,39 +95,31 @@ async def test_get_config_for_ref_dot_github( "githubConfigFile": { "text": "# .github/.kodiak.toml\nversion = 1\nmerge.method = 'rebase'" }, - "refs": { - "edges": [ - { - "node": { - "branchProtectionRule": { - "matchingRefs": { - "nodes": [{"name": "master"}] - }, - "requiresApprovingReviews": True, - "requiredApprovingReviewCount": 2, - "requiresStatusChecks": True, - "requiredStatusCheckContexts": [ - "ci/circleci: backend_lint", - "ci/circleci: backend_test", - "ci/circleci: frontend_lint", - "ci/circleci: frontend_test", - "WIP (beta)", - ], - "requiresStrictStatusChecks": True, - "requiresCodeOwnerReviews": False, - "requiresCommitSignatures": False, - "requiresConversationResolution": False, - "restrictsPushes": True, - "pushAllowances": { - "nodes": [ - {"actor": {}}, - {"actor": {"databaseId": 53453}}, - ] - }, - } - } - } - ] + "ref": { + "branchProtectionRule": { + "matchingRefs": {"nodes": [{"name": "master"}]}, + "requiresApprovingReviews": True, + "requiredApprovingReviewCount": 2, + "requiresStatusChecks": True, + "requiredStatusCheckContexts": [ + "ci/circleci: backend_lint", + "ci/circleci: backend_test", + "ci/circleci: frontend_lint", + "ci/circleci: frontend_test", + "WIP (beta)", + ], + "requiresStrictStatusChecks": True, + "requiresCodeOwnerReviews": False, + "requiresCommitSignatures": False, + "requiresConversationResolution": False, + "restrictsPushes": True, + "pushAllowances": { + "nodes": [ + {"actor": {}}, + {"actor": {"databaseId": 53453}}, + ] + }, + } }, } } From dc2a07f2638e52de5269198cceb8821e5906d0f1 Mon Sep 17 00:00:00 2001 From: Christopher Dignam Date: Wed, 19 Jun 2024 21:56:31 -0400 Subject: [PATCH 4/5] . --- bot/kodiak/test_queries.py | 1 - 1 file changed, 1 deletion(-) diff --git a/bot/kodiak/test_queries.py b/bot/kodiak/test_queries.py index ca35478b8..8ba38cc22 100644 --- a/bot/kodiak/test_queries.py +++ b/bot/kodiak/test_queries.py @@ -97,7 +97,6 @@ async def test_get_config_for_ref_dot_github( }, "ref": { "branchProtectionRule": { - "matchingRefs": {"nodes": [{"name": "master"}]}, "requiresApprovingReviews": True, "requiredApprovingReviewCount": 2, "requiresStatusChecks": True, From 506460b33700ea963dc4ca90ac45c99eb439ee24 Mon Sep 17 00:00:00 2001 From: Christopher Dignam Date: Wed, 19 Jun 2024 22:06:20 -0400 Subject: [PATCH 5/5] fmt --- bot/kodiak/queries/__init__.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/bot/kodiak/queries/__init__.py b/bot/kodiak/queries/__init__.py index 396109778..3b98d967b 100644 --- a/bot/kodiak/queries/__init__.py +++ b/bot/kodiak/queries/__init__.py @@ -630,7 +630,9 @@ def get_branch_protection( *, config_response: Dict[str, Any], ref_name: str ) -> Optional[BranchProtectionRule]: try: - branchProtectionRule = config_response["repository"]["ref"]["branchProtectionRule"] + branchProtectionRule = config_response["repository"]["ref"][ + "branchProtectionRule" + ] try: return BranchProtectionRule.parse_obj(branchProtectionRule) except ValueError: