Re-use the github PR object when fetching reviews

We inadvertently fetch the PR object twice because of the way
we were fetching reviews.  Keep it around so we can make one
less API call.

Co-Authored-By: Clark Boylan <cboylan@sapwetik.org>
Change-Id: If5260278adb525566d99eedaecaf8b4f5077d43e

tests/base.py

@@ -784,6 +784,15 @@ class GithubChangeReference(git.Reference):
     _points_to_commits_only = True
 
 
+class FakeGHReview(object):
+
+    def __init__(self, data):
+        self.data = data
+
+    def as_dict(self):
+        return self.data
+
+
 class FakeGithubPullRequest(object):
 
     def __init__(self, github, number, project, branch,
@@ -1050,14 +1059,14 @@ class FakeGithubPullRequest(object):
             submitted_at = time.strftime(
                 gh_time_format, granted_on.timetuple())
 
-        self.reviews.append({
+        self.reviews.append(FakeGHReview({
             'state': state,
             'user': {
                 'login': user,
                 'email': user + "@derp.com",
             },
             'submitted_at': submitted_at,
-        })
+        }))
 
     def getPRReference(self):
         return '%s/head' % self.number
@@ -1229,10 +1238,6 @@ class FakeGithubConnection(githubconnection.GithubConnection):
         super(FakeGithubConnection, self).addProject(project)
         self.getGithubClient(project).addProject(project)
 
-    def _getPullReviews(self, owner, project, number):
-        pr = self.pull_requests[int(number)]
-        return pr.reviews
-
     def getGitUrl(self, project):
         if self.git_url_with_auth:
             auth_token = ''.join(

tests/fakegithub.py

@@ -296,6 +296,9 @@ class FakePull(object):
         return [FakeFile(fn)
                 for fn in sorted(self._fake_pull_request.files)][:300]
 
+    def reviews(self):
+        return self._fake_pull_request.reviews
+
     @property
     def head(self):
         client = FakeGithubClient(self._fake_pull_request.github.github_data)

zuul/driver/github/githubconnection.py

@@ -417,7 +417,8 @@ class GithubEventProcessor(object):
     def _issue_to_pull_request(self, body):
         number = body.get('issue').get('number')
         project_name = body.get('repository').get('full_name')
-        pr_body = self.connection.getPull(project_name, number, self.log)
+        pr_body, pr_obj = self.connection.getPull(
+            project_name, number, self.log)
         if pr_body is None:
             self.log.debug('Pull request #%s not found in project %s' %
                            (number, project_name))
@@ -963,7 +964,7 @@ class GithubConnection(BaseConnection):
 
     def _updateChange(self, change):
         self.log.info("Updating %s" % (change,))
-        change.pr = self.getPull(change.project.name, change.number)
+        change.pr, pr_obj = self.getPull(change.project.name, change.number)
         change.ref = "refs/pull/%s/head" % change.number
         change.branch = change.pr.get('base').get('ref')
 
@@ -989,7 +990,7 @@ class GithubConnection(BaseConnection):
         change.is_merged = change.pr.get('merged')
         change.status = self._get_statuses(change.project,
                                            change.patchset)
-        change.reviews = self.getPullReviews(change.project,
+        change.reviews = self.getPullReviews(pr_obj, change.project,
                                              change.number)
         change.labels = change.pr.get('labels')
         # ensure message is at least an empty string
@@ -1151,7 +1152,7 @@ class GithubConnection(BaseConnection):
 
         log.debug('Got PR %s#%s', project_name, number)
         self.log_rate_limit(self.log, github)
-        return pr
+        return (pr, probj)
 
     def canMerge(self, change, allow_needs):
         # NOTE: The mergeable call may get a false (null) while GitHub is
@@ -1191,7 +1192,8 @@ class GithubConnection(BaseConnection):
             raise Exception('Multiple pulls found with head sha %s' % sha)
         if len(cached_pr_numbers) == 1:
             for pr in cached_pr_numbers:
-                return self.getPull(project, pr, log)
+                pr_body, pr_obj = self.getPull(project, pr, log)
+                return pr_body
 
         pulls = []
         project_name = project
@@ -1216,10 +1218,11 @@ class GithubConnection(BaseConnection):
             return None
         return pulls.pop()
 
-    def getPullReviews(self, project, number):
-        owner, proj = project.name.split('/')
-
-        revs = self._getPullReviews(owner, proj, number)
+    def getPullReviews(self, pr_obj, project, number):
+        # make a list out of the reviews so that we complete our
+        # API transaction
+        revs = [review.as_dict() for review in pr_obj.reviews()]
+        self.log.debug('Got reviews for PR %s#%s', project, number)
 
         permissions = {}
         reviews = {}
@@ -1325,17 +1328,6 @@ class GithubConnection(BaseConnection):
         # we allow additional successful status contexts we don't care about.
         return required_contexts.issubset(successful)
 
-    def _getPullReviews(self, owner, project, number):
-        # make a list out of the reviews so that we complete our
-        # API transaction
-        github = self.getGithubClient("%s/%s" % (owner, project))
-        reviews = [review.as_dict() for review in
-                   github.pull_request(owner, project, number).reviews()]
-
-        self.log.debug('Got reviews for PR %s/%s#%s', owner, project, number)
-        self.log_rate_limit(self.log, github)
-        return reviews
-
     def getUser(self, login, project):
         return GithubUser(login, self, project)
 

zuul/driver/github/githubsource.py

@@ -81,7 +81,7 @@ class GithubSource(BaseSource):
             num = int(m.group(3))
         except ValueError:
             return None
-        pull = self.connection.getPull('%s/%s' % (org, proj), int(num))
+        pull, pr_obj = self.connection.getPull('%s/%s' % (org, proj), int(num))
         if not pull:
             return None
         proj = pull.get('base').get('repo').get('full_name')