From 0093aabd4eb3210bfe123c23240c4c03edbd8321 Mon Sep 17 00:00:00 2001 From: Tobias Henkel Date: Thu, 16 Jul 2020 10:26:15 +0200 Subject: [PATCH] Fix multiple prs found when commit is not head When receiving a status or checks event we need to find the corresponding pull request to that event. Github stores the status on the head commit of a pr and not on the pr itself which leads to problems if multiple prs exist with the same head but different target branches. Therefore zuul errors out when more than one pr is found for that event. However the github search also returns all prs that contain the sha we search for but have a different head which leads to the same error as if we had two conflicting prs. This can be solved by delaying the error and filtering the pr bodies for the head sha first. Change-Id: Iadafd3cf68a742941e9189e84fca594bc3394084 --- tests/base.py | 16 ++++++++++------ tests/fakegithub.py | 21 ++++++++++++++++++--- tests/unit/test_github_requirements.py | 5 +++++ zuul/driver/github/githubconnection.py | 23 ++++++++++++++++------- 4 files changed, 49 insertions(+), 16 deletions(-) diff --git a/tests/base.py b/tests/base.py index 8cbe7a6c6c..7c063e68c8 100644 --- a/tests/base.py +++ b/tests/base.py @@ -1909,7 +1909,8 @@ class FakeGithubPullRequest(object): def __init__(self, github, number, project, branch, subject, upstream_root, files=[], number_of_commits=1, - writers=[], body=None, body_text=None, draft=False): + writers=[], body=None, body_text=None, draft=False, + base_sha=None): """Creates a new PR with several commits. Sends an event about opened PR.""" self.github = github @@ -1936,7 +1937,7 @@ class FakeGithubPullRequest(object): self.merge_message = None self.state = 'open' self.url = 'https://%s/%s/pull/%s' % (github.server, project, number) - self._createPRRef() + self._createPRRef(base_sha=base_sha) self._addCommitToRepo(files=files) self._updateTimeStamp() @@ -2103,10 +2104,11 @@ class FakeGithubPullRequest(object): repo_path = os.path.join(self.upstream_root, self.project) return git.Repo(repo_path) - def _createPRRef(self): + def _createPRRef(self, base_sha=None): + base_sha = base_sha or 'refs/tags/init' repo = self._getRepo() GithubChangeReference.create( - repo, self.getPRReference(), 'refs/tags/init') + repo, self.getPRReference(), base_sha) def _addCommitToRepo(self, files=[], reset=False): repo = self._getRepo() @@ -2354,11 +2356,13 @@ class FakeGithubConnection(githubconnection.GithubConnection): self.zuul_web_port = port def openFakePullRequest(self, project, branch, subject, files=[], - body=None, body_text=None, draft=False): + body=None, body_text=None, draft=False, + base_sha=None): self.pr_number += 1 pull_request = FakeGithubPullRequest( self, self.pr_number, project, branch, subject, self.upstream_root, - files=files, body=body, body_text=body_text, draft=draft) + files=files, body=body, body_text=body_text, draft=draft, + base_sha=base_sha) self.pull_requests[self.pr_number] = pull_request return pull_request diff --git a/tests/fakegithub.py b/tests/fakegithub.py index 28f0203eaa..ec99832e48 100644 --- a/tests/fakegithub.py +++ b/tests/fakegithub.py @@ -687,9 +687,24 @@ class FakeGithubClient(object): return re.match(r'[a-z0-9]{40}', s) if query_is_sha(query): - return (FakeIssueSearchResult(FakeIssue(pr)) - for pr in self._data.pull_requests.values() - if pr.head_sha == query) + # Github returns all PR's that contain the sha in their history + result = [] + for pr in self._data.pull_requests.values(): + # Quick check if head sha matches + if pr.head_sha == query: + result.append(FakeIssueSearchResult(FakeIssue(pr))) + continue + + # If head sha doesn't match it still could be in the pr history + repo = pr._getRepo() + commits = repo.iter_commits( + '%s...%s' % (pr.branch, pr.head_sha)) + for commit in commits: + if commit.hexsha == query: + result.append(FakeIssueSearchResult(FakeIssue(pr))) + continue + + return result # Non-SHA queries are of the form: # diff --git a/tests/unit/test_github_requirements.py b/tests/unit/test_github_requirements.py index e542f80f96..bde2c5ca8d 100644 --- a/tests/unit/test_github_requirements.py +++ b/tests/unit/test_github_requirements.py @@ -92,6 +92,11 @@ class TestGithubRequirements(ZuulTestCase): project = 'org/project2' A = self.fake_github.openFakePullRequest(project, 'master', 'A') + # Create second PR which contains the head of A in its history. Zuul + # should not get disturbed by the existence of this one. + self.fake_github.openFakePullRequest( + project, 'master', 'A', base_sha=A.head_sha) + # An error status should not cause it to be enqueued self.fake_github.setCommitStatus(project, A.head_sha, 'error', context='tenant-one/check') diff --git a/zuul/driver/github/githubconnection.py b/zuul/driver/github/githubconnection.py index da242401ee..514e0c087d 100644 --- a/zuul/driver/github/githubconnection.py +++ b/zuul/driver/github/githubconnection.py @@ -1660,16 +1660,25 @@ class GithubConnection(BaseConnection): issues = list(github.search_issues(sha)) log.debug('Got PR on project %s for sha %s', project_name, sha) - if len(issues) > 1: - raise Exception('Multiple pulls found with head sha %s' % sha) - if len(issues) == 0: return None - pr_body = self._getChange( - project, issues.pop().issue.number, sha, event=event).pr - self._sha_pr_cache.update(project_name, pr_body) - return pr_body + # Github returns all issues that contain the sha, not only the ones + # with that sha as head_sha so we need to get and update all those + # changes and then filter for the head sha before we can error out + # with multiple pulls found. + found_pr_body = None + for item in issues: + pr_body = self._getChange( + project, item.issue.number, sha, event=event).pr + self._sha_pr_cache.update(project_name, pr_body) + if pr_body['head']['sha'] == sha: + if found_pr_body: + raise Exception( + 'Multiple pulls found with head sha %s' % sha) + found_pr_body = pr_body + + return found_pr_body def getPullReviews(self, pr_obj, project, number, event): log = get_annotated_logger(self.log, event)