zuul/zuul
Code Issues Proposed changes

Support github required conversation resolution

Browse Source 
As part of its branch protection rules, github has the option to
require that all conversations be "resolved".  Resolving is an
explicit user action and doing so sets a flag on the review thread.

("Conversation" in the UI seems to be a synonym for "review thread"
in the API.)

Unfortunately, whether all conversations are resolved is not exposed
on the pull request as a boolean (like "reviewDecision").  So to
determine if this is the case, we will fetch the review threads as
part of the canMerge query.  In the unlikely event there are more
than 100 threads, we will use pagination to retrieve the remainder
in subsequent queries.  Though as soon as we find the first
unresolved conversation, we will stop.

(It may be possible to infer this state, perhaps along with other
boolean blockers, by examining the mergeStateStatus on the PR.
However, this would likely require some siginificant real-world
data collection to determine if it is sufficiently reliable for
the task.  That is left to a future change.)

Change-Id: I1b4907ad1837548a9ec65a5d5e15b06f57fcdd45
This commit is contained in:
James E. Blair 2024-09-18 11:26:40 -07:00
parent 108977ca47
commit 8dd39c1c8e
8 changed files with 199 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

48
tests/fake_graphql.py
View File

@ -42,6 +42,8 @@ class Node(Interface):
return Commit
elif kind == 'CheckSuite':
return CheckSuite
elif kind == 'PullRequest':
return PullRequest
class PageInfo(ObjectType):
@ -86,6 +88,7 @@ class BranchProtectionRule(ObjectType):
pattern = String()
requiredStatusCheckContexts = List(String)
requiresApprovingReviews = Boolean()
requiresConversationResolution = Boolean()
requiresCodeOwnerReviews = Boolean()
matchingRefs = Field(MatchingRefs, first=Int(), after=String(),
query=String())
@ -103,6 +106,9 @@ class BranchProtectionRule(ObjectType):
def resolve_requiresApprovingReviews(parent, info):
return parent.require_reviews
def resolve_requiresConversationResolution(parent, info):
return parent.require_conversation_resolution
def resolve_requiresCodeOwnerReviews(parent, info):
return parent.require_codeowners_review
@ -277,10 +283,37 @@ class Commit(ObjectType):
)
class PullRequestReviewThread(ObjectType):
isResolved = Boolean()
def resolve_isResolved(parent, info):
return parent.resolved
class PullRequestReviewThreads(ObjectType):
nodes = List(PullRequestReviewThread)
pageInfo = Field(PageInfo)
def resolve_nodes(parent, info):
return parent['nodes']
def resolve_pageInfo(parent, info):
return parent
class PullRequest(ObjectType):
class Meta:
interfaces = (Node, )
id = ID(required=True)
isDraft = Boolean()
reviewDecision = String()
mergeable = String()
reviewThreads = Field(PullRequestReviewThreads,
first=Int(), after=String())
def resolve_id(parent, info):
return parent.id
def resolve_isDraft(parent, info):
return parent.draft
@ -288,6 +321,18 @@ class PullRequest(ObjectType):
def resolve_mergeable(parent, info):
return "MERGEABLE" if parent.mergeable else "CONFLICTING"
def resolve_reviewThreads(parent, info, first, after=None):
if after is None:
after = '0'
after = int(after)
values = parent.review_threads
return dict(
length=len(values),
nodes=values[after:after + first],
first=first,
after=after,
)
def resolve_reviewDecision(parent, info):
if hasattr(info.context, 'version') and info.context.version:
if info.context.version < (2, 21, 0):
@ -358,6 +403,9 @@ class FakeGithubQuery(ObjectType):
for suite in commit._check_suites.values():
if suite.id == id:
return suite
for pr in info.context._data.pull_requests.values():
if pr.id == id:
return pr
def getGrapheneSchema():

36
tests/fakegithub.py
View File

@ -45,7 +45,8 @@ class GithubChangeReference(git.Reference):
_points_to_commits_only = True
class FakeGithubPullRequest(object):
class FakeGithubPullRequest:
_graphene_type = 'PullRequest'
def __init__(self, github, number, project, branch,
subject, upstream_root, files=None, number_of_commits=1,
@ -57,6 +58,7 @@ class FakeGithubPullRequest(object):
If the `files` argument is provided it must be a dictionary of
file names OR FakeFile instances -> content.
"""
self.id = str(uuid.uuid4())
self.source_hostname = github.canonical_hostname
self.github_server = github.server
self.number = number
@ -75,6 +77,7 @@ class FakeGithubPullRequest(object):
self.labels = []
self.statuses = {}
self.reviews = []
self.review_threads = []
self.writers = []
self.admins = []
self.updated_at = None
@ -343,14 +346,24 @@ class FakeGithubPullRequest(object):
submitted_at = time.strftime(
gh_time_format, granted_on.timetuple())
self.reviews.append(FakeGHReview({
review = FakeGithubReview({
'state': state,
'user': {
'login': user,
'email': user + "@example.com",
},
'submitted_at': submitted_at,
}))
})
self.reviews.append(review)
return review
def addReviewThread(self, review):
# Create and return a new list of reviews which constitute a
# thread
thread = FakeGithubReviewThread()
thread.addReview(review)
self.review_threads.append(thread)
return thread
def getPRReference(self):
return '%s/head' % self.number
@ -579,7 +592,7 @@ class FakeCheckRun:
self.status = "completed"
class FakeGHReview(object):
class FakeGithubReview(object):
def __init__(self, data):
self.data = data
@ -588,6 +601,16 @@ class FakeGHReview(object):
return self.data
class FakeGithubReviewThread(object):
def __init__(self):
self.resolved = False
self.reviews = []
def addReview(self, review):
self.reviews.append(review)
class FakeCombinedStatus(object):
def __init__(self, sha, statuses):
self.sha = sha
@ -698,6 +721,7 @@ class FakeRepository(object):
def _set_branch_protection(self, branch_name, protected=True,
contexts=None, require_review=False,
require_conversation_resolution=False,
locked=False):
if not protected:
if branch_name in self._branch_protection_rules:
@ -708,6 +732,7 @@ class FakeRepository(object):
rule.pattern = branch_name
rule.required_contexts = contexts or []
rule.require_reviews = require_review
rule.require_conversation_resolution = require_conversation_resolution
rule.matching_refs = [branch_name]
rule.lock_branch = locked
return rule
@ -955,7 +980,7 @@ class FakePull(object):
return self._fake_pull_request.reviews
def create_review(self, body, commit_id, event):
review = FakeGHReview({
review = FakeGithubReview({
'state': event,
'user': {
'login': 'fakezuul',
@ -1218,6 +1243,7 @@ class FakeBranchProtectionRule:
self.pattern = None
self.required_contexts = []
self.require_reviews = False
self.require_conversation_resolution = False
self.require_codeowners_review = False

54
tests/unit/test_github_driver.py
View File

@ -404,6 +404,30 @@ class TestGithubDriver(ZuulTestCase):
self.assertEqual(1, len(C.reviews))
self.assertEqual('APPROVE', C.reviews[0].as_dict()['state'])
@simple_layout('layouts/gate-github.yaml', driver='github')
def test_conversations(self):
project = self.fake_github.getProject('org/project')
github = self.fake_github.getGithubClient(project.name)
repo = github.repo_from_project('org/project')
repo._set_branch_protection('master',
require_conversation_resolution=True,
protected=True)
A = self.fake_github.openFakePullRequest('org/project', 'master', 'A')
review = A.addReview('user', 'COMMENTED')
thread = A.addReviewThread(review)
self.fake_github.emitEvent(A.getPullRequestOpenedEvent())
self.waitUntilSettled()
self.assertHistory([])
thread.resolved = True
self.fake_github.emitEvent(A.getPullRequestOpenedEvent())
self.waitUntilSettled()
self.assertHistory([
dict(name='project-test1', result='SUCCESS'),
dict(name='project-test2', result='SUCCESS'),
], ordered=False)
@simple_layout('layouts/basic-github.yaml', driver='github')
def test_timer_event(self):
self.executor_server.hold_jobs_in_build = True
@ -2927,3 +2951,33 @@ class TestGithubGraphQL(ZuulTestCase):
# We should have one run from each app; the duplicate runs are
# dropped by github.
self.assertEqual(num_apps + 1, len(result['checks'].keys()))
@simple_layout('layouts/basic-github.yaml', driver='github')
def test_graphql_query_canmerge_conversations(self):
project = self.fake_github.getProject('org/project')
github = self.fake_github.getGithubClient(project.name)
repo = github.repo_from_project('org/project')
repo._set_branch_protection('master',
require_conversation_resolution=True,
protected=True)
A = self.fake_github.openFakePullRequest('org/project', 'master', 'A')
num_threads = 102
for thread_no in range(num_threads):
review = A.addReview('user', 'COMMENTED')
thread = A.addReviewThread(review)
thread.resolved = True
# Since we short circuit on the first unresolved thread, mark
# all of them as resolved except the last.
thread.resolved = False
conn = self.scheds.first.sched.connections.connections["github"]
change_key = ChangeKey(conn.connection_name, "org/project",
"PullRequest", str(A.number), str(A.head_sha))
change = conn.getChange(change_key, refresh=True)
result = self.fake_github.graphql_client.fetch_canmerge(
github, change)
self.assertTrue(result['protected'])
self.assertTrue(result['requiresConversationResolution'])
self.assertTrue(result['unresolvedConversations'])

8
zuul/driver/github/githubconnection.py
View File

@ -1782,6 +1782,8 @@ class GithubConnection(ZKChangeCacheMixin, ZKBranchCacheMixin, BaseConnection):
change.mergeable = (canmerge_data.get('mergeable', 'MERGEABLE').lower()
in ('mergeable', 'unknown'))
change.review_decision = canmerge_data['reviewDecision']
change.unresolved_conversations = canmerge_data.get(
'unresolvedConversations', False)
change.required_contexts = set(
canmerge_data['requiredStatusCheckContexts']
)
@ -2086,6 +2088,12 @@ class GithubConnection(ZKChangeCacheMixin, ZKBranchCacheMixin, BaseConnection):
change)
return False
if change.unresolved_conversations:
log.debug('Change %s can not merge because '
'it has unresolved conversations',
change)
return False
return True
def getPullBySha(self, sha, project_name, event):

3
zuul/driver/github/githubmodel.py
View File

@ -40,6 +40,7 @@ class PullRequest(Change):
self.draft = None
self.mergeable = True
self.review_decision = None
self.unresolved_conversations = None
self.required_contexts = set()
self.contexts = set()
self.branch_protected = False
@ -77,6 +78,7 @@ class PullRequest(Change):
"draft": self.draft,
"mergeable": self.mergeable,
"review_decision": self.review_decision,
"unresolved_conversations": self.unresolved_conversations,
"required_contexts": list(self.required_contexts),
"contexts": list(self.contexts),
"branch_protected": self.branch_protected,
@ -94,6 +96,7 @@ class PullRequest(Change):
self.draft = data.get("draft")
self.mergeable = data.get("mergeable", True)
self.review_decision = data.get("review_decision")
self.unresolved_conversations = data.get("unresolved_conversations")
self.required_contexts = set(data.get("required_contexts", []))
self.contexts = set(tuple(c) for c in data.get("contexts", []))
self.branch_protected = data.get("branch_protected", False)

26
zuul/driver/github/graphql/__init__.py
View File

@ -41,6 +41,7 @@ class GraphQLClient:
'canmerge-page-rules',
'canmerge-page-suites',
'canmerge-page-runs',
'canmerge-page-threads',
'branch-protection',
'branch-protection-inner',
]
@ -134,6 +135,8 @@ class GraphQLClient:
'requiresApprovingReviews')
result['requiresCodeOwnerReviews'] = matching_rule.get(
'requiresCodeOwnerReviews')
result['requiresConversationResolution'] = matching_rule.get(
'requiresConversationResolution')
result['protected'] = True
else:
result['requiredStatusCheckContexts'] = []
@ -153,6 +156,14 @@ class GraphQLClient:
result['reviewDecision'] = nested_get(
pull_request, 'reviewDecision', default=None)
if result.get('requiresConversationResolution'):
result['unresolvedConversations'] = False
for thread in self._fetch_canmerge_threads(
github, log, pull_request):
if not thread.get('isResolved'):
result['unresolvedConversations'] = True
break
# Add status checks
result['status'] = {}
commit = nested_get(repository, 'object')
@ -202,6 +213,21 @@ class GraphQLClient:
cursor=page_info['endCursor'])
suite = nested_get(data, 'data', 'node')
def _fetch_canmerge_threads(self, github, log, pull_request):
pull_id = pull_request['id']
while True:
for thread in nested_get(
pull_request, 'reviewThreads', 'nodes', default=[]):
yield thread
page_info = nested_get(pull_request, 'reviewThreads', 'pageInfo')
if not page_info['hasNextPage']:
return
data = self._run_query(
log, github, 'canmerge-page-threads',
pull_node_id=pull_id,
cursor=page_info['endCursor'])
pull_request = nested_get(data, 'data', 'node')
def _fetch_branch_protection(self, log, github, project,
zuul_event_id=None):
owner, repo = project.name.split('/')

18
zuul/driver/github/graphql/canmerge-page-threads.graphql Normal file
View File

@ -0,0 +1,18 @@
query canMergeDataPageThreads(
$pull_node_id: ID!
$cursor: String
) {
node(id: $pull_node_id) {
... on PullRequest {
reviewThreads(first: 100, after: $cursor) {
pageInfo {
endCursor
hasNextPage
}
nodes {
isResolved
}
}
}
}
}

11
zuul/driver/github/graphql/canmerge.graphql
View File

@ -16,6 +16,7 @@ query canMergeData(
requiredStatusCheckContexts
requiresApprovingReviews
requiresCodeOwnerReviews
requiresConversationResolution
matchingRefs(first: 100, query: $ref_name) {
nodes {
name
@ -24,9 +25,19 @@ query canMergeData(
}
}
pullRequest(number: $pull) {
id
isDraft
mergeable
reviewDecision
reviewThreads(first: 100) {
pageInfo {
endCursor
hasNextPage
}
nodes {
isResolved
}
}
}
object(expression: $head_sha) {
... on Commit {