Merge pull request #5985 from saadmk11/build-status-bug-fix

ericholscher · web-flow · commit 9ff43dfdfcb7 · 2019-07-31T08:45:13.000-07:00
Fix github build status reporting bug
diff --git a/readthedocs/api/v2/views/integrations.py b/readthedocs/api/v2/views/integrations.py
@@ -219,7 +219,9 @@ def get_external_version_response(self, project):
             project, identifier, verbose_name
         )
         # returns external version verbose_name (pull/merge request number)
-        to_build = build_external_version(project, external_version)
+        to_build = build_external_version(
+            project=project, version=external_version, commit=identifier
+        )
 
         return {
             'build_triggered': True,
diff --git a/readthedocs/core/utils/__init__.py b/readthedocs/core/utils/__init__.py
@@ -60,6 +60,7 @@ def broadcast(type, task, args, kwargs=None, callback=None):  # pylint: disable=
 def prepare_build(
         project,
         version=None,
+        commit=None,
         record=True,
         force=False,
         immutable=True,
@@ -72,6 +73,7 @@ def prepare_build(
 
     :param project: project's documentation to be built
     :param version: version of the project to be built. Default: ``project.get_default_version()``
+    :param commit: commit sha of the version required for sending build status reports
     :param record: whether or not record the build in a new Build object
     :param force: build the HTML documentation even if the files haven't changed
     :param immutable: whether or not create an immutable Celery signature
@@ -102,6 +104,7 @@ def prepare_build(
     kwargs = {
         'record': record,
         'force': force,
+        'commit': commit,
     }
 
     if record:
@@ -111,6 +114,7 @@ def prepare_build(
             type='html',
             state=BUILD_STATE_TRIGGERED,
             success=True,
+            commit=commit
         )
         kwargs['build_pk'] = build.pk
 
@@ -131,9 +135,12 @@ def prepare_build(
     options['soft_time_limit'] = time_limit
     options['time_limit'] = int(time_limit * 1.2)
 
-    if build:
+    if build and commit:
         # Send pending Build Status using Git Status API for External Builds.
-        send_external_build_status(version=version, build_pk=build.id, status=BUILD_STATUS_PENDING)
+        send_external_build_status(
+            version_type=version.type, build_pk=build.id,
+            commit=commit, status=BUILD_STATUS_PENDING
+        )
 
     return (
         update_docs_task.signature(
@@ -146,7 +153,7 @@ def prepare_build(
     )
 
 
-def trigger_build(project, version=None, record=True, force=False):
+def trigger_build(project, version=None, commit=None, record=True, force=False):
     """
     Trigger a Build.
 
@@ -155,6 +162,7 @@ def trigger_build(project, version=None, record=True, force=False):
 
     :param project: project's documentation to be built
     :param version: version of the project to be built. Default: ``latest``
+    :param commit: commit sha of the version required for sending build status reports
     :param record: whether or not record the build in a new Build object
     :param force: build the HTML documentation even if the files haven't changed
     :returns: Celery AsyncResult promise and Build instance
@@ -163,6 +171,7 @@ def trigger_build(project, version=None, record=True, force=False):
     update_docs_task, build = prepare_build(
         project,
         version,
+        commit,
         record,
         force,
         immutable=True,
diff --git a/readthedocs/core/views/hooks.py b/readthedocs/core/views/hooks.py
@@ -157,7 +157,7 @@ def delete_external_version(project, identifier, verbose_name):
     return None
 
 
-def build_external_version(project, version):
+def build_external_version(project, version, commit):
     """
     Where we actually trigger builds for external versions.
 
@@ -173,6 +173,6 @@ def build_external_version(project, version):
         project.slug,
         version.slug,
     )
-    trigger_build(project=project, version=version, force=True)
+    trigger_build(project=project, version=version, commit=commit, force=True)
 
     return version.verbose_name
diff --git a/readthedocs/oauth/services/base.py b/readthedocs/oauth/services/base.py
@@ -250,12 +250,14 @@ def update_webhook(self, project, integration):
         """
         raise NotImplementedError
 
-    def send_build_status(self, build, state):
+    def send_build_status(self, build, commit, state):
         """
         Create commit status for project.
 
         :param build: Build to set up commit status for
         :type build: Build
+        :param commit: commit sha of the pull/merge request
+        :type commit: str
         :param state: build state failure, pending, or success.
         :type state: str
         :returns: boolean based on commit status creation was successful or not.
diff --git a/readthedocs/oauth/services/github.py b/readthedocs/oauth/services/github.py
@@ -315,21 +315,22 @@ def update_webhook(self, project, integration):
             )
             return (False, resp)
 
-    def send_build_status(self, build, state):
+    def send_build_status(self, build, commit, state):
         """
         Create GitHub commit status for project.
 
         :param build: Build to set up commit status for
         :type build: Build
         :param state: build state failure, pending, or success.
         :type state: str
+        :param commit: commit sha of the pull request
+        :type commit: str
         :returns: boolean based on commit status creation was successful or not.
         :rtype: Bool
         """
         session = self.get_session()
         project = build.project
         owner, repo = build_utils.get_github_username_repo(url=project.repo)
-        build_sha = build.version.identifier
 
         # select the correct state and description.
         github_build_state = SELECT_BUILD_STATUS[state]['github']
@@ -346,7 +347,7 @@ def send_build_status(self, build, state):
 
         try:
             resp = session.post(
-                f'https://api.github.com/repos/{owner}/{repo}/statuses/{build_sha}',
+                f'https://api.github.com/repos/{owner}/{repo}/statuses/{commit}',
                 data=json.dumps(data),
                 headers={'content-type': 'application/json'},
             )
diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py
@@ -145,7 +145,8 @@ def sync_repo(self):
         version_repo = self.get_vcs_repo()
         version_repo.update()
         self.sync_versions(version_repo)
-        version_repo.checkout(self.version.identifier)
+        identifier = self.commit or self.version.identifier
+        version_repo.checkout(identifier)
 
     def sync_versions(self, version_repo):
         """
@@ -322,6 +323,7 @@ def __init__(
             build=None,
             project=None,
             version=None,
+            commit=None,
             task=None,
     ):
         self.build_env = build_env
@@ -333,6 +335,7 @@ def __init__(
         self.version = {}
         if version is not None:
             self.version = version
+        self.commit = commit
         self.project = {}
         if project is not None:
             self.project = project
@@ -343,7 +346,7 @@ def __init__(
 
     # pylint: disable=arguments-differ
     def run(
-            self, version_pk, build_pk=None, record=True, docker=None,
+            self, version_pk, build_pk=None, commit=None, record=True, docker=None,
             force=False, **__
     ):
         """
@@ -364,6 +367,7 @@ def run(
 
         :param version_pk int: Project Version id
         :param build_pk int: Build id (if None, commands are not recorded)
+        :param commit: commit sha of the version required for sending build status reports
         :param record bool: record a build object in the database
         :param docker bool: use docker to build the project (if ``None``,
             ``settings.DOCKER_ENABLE`` is used)
@@ -380,6 +384,7 @@ def run(
             self.project = self.version.project
             self.build = self.get_build(build_pk)
             self.build_force = force
+            self.commit = commit
             self.config = None
 
             # Build process starts here
@@ -586,27 +591,42 @@ def run_build(self, docker, record):
                     log.warning('No build ID, not syncing files')
 
         if self.build_env.failed:
+            # TODO: Send RTD Webhook notification for build failure.
             self.send_notifications(self.version.pk, self.build['id'])
-            send_external_build_status(
-                version=self.version, build_pk=self.build['id'], status=BUILD_STATUS_FAILURE
-            )
+
+            if self.commit:
+                send_external_build_status(
+                    version_type=self.version.type,
+                    build_pk=self.build['id'],
+                    commit=self.commit,
+                    status=BUILD_STATUS_FAILURE
+                )
         elif self.build_env.successful:
-            send_external_build_status(
-                version=self.version, build_pk=self.build['id'], status=BUILD_STATUS_SUCCESS
-            )
+            # TODO: Send RTD Webhook notification for build success.
+            if self.commit:
+                send_external_build_status(
+                    version_type=self.version.type,
+                    build_pk=self.build['id'],
+                    commit=self.commit,
+                    status=BUILD_STATUS_SUCCESS
+                )
         else:
-            msg = 'Unhandled Build Status'
-            send_external_build_status(
-                version=self.version, build_pk=self.build['id'], status=BUILD_STATUS_FAILURE
-            )
-            log.warning(
-                LOG_TEMPLATE,
-                {
-                    'project': self.project.slug,
-                    'version': self.version.slug,
-                    'msg': msg,
-                }
-            )
+            if self.commit:
+                msg = 'Unhandled Build Status'
+                send_external_build_status(
+                    version_type=self.version.type,
+                    build_pk=self.build['id'],
+                    commit=self.commit,
+                    status=BUILD_STATUS_FAILURE
+                )
+                log.warning(
+                    LOG_TEMPLATE,
+                    {
+                        'project': self.project.slug,
+                        'version': self.version.slug,
+                        'msg': msg,
+                    }
+                )
 
         build_complete.send(sender=Build, build=self.build_env.build)
 
@@ -675,7 +695,7 @@ def setup_vcs(self):
             # Re raise the exception to stop the build at this point
             raise
 
-        commit = self.project.vcs_repo(self.version.slug).commit
+        commit = self.commit or self.project.vcs_repo(self.version.slug).commit
         if commit:
             self.build['commit'] = commit
 
@@ -1859,11 +1879,12 @@ def retry_domain_verification(domain_pk):
 
 
 @app.task(queue='web')
-def send_build_status(build_pk, status):
+def send_build_status(build_pk, commit, status):
     """
     Send Build Status to Git Status API for project external versions.
 
     :param build_pk: Build primary key
+    :param commit: commit sha of the pull/merge request
     :param status: build status failed, pending, or success to be sent.
     """
     build = Build.objects.get(pk=build_pk)
@@ -1875,7 +1896,7 @@ def send_build_status(build_pk, status):
             )
 
             # send Status report using the API.
-            service.send_build_status(build, status)
+            service.send_build_status(build, commit, status)
 
     except RemoteRepository.DoesNotExist:
         log.info('Remote repository does not exist for %s', build.project)
@@ -1886,16 +1907,17 @@ def send_build_status(build_pk, status):
     # TODO: Send build status for other providers.
 
 
-def send_external_build_status(version, build_pk, status):
+def send_external_build_status(version_type, build_pk, commit, status):
     """
     Check if build is external and Send Build Status for project external versions.
 
-     :param version: Version instance
+     :param version_type: Version type e.g EXTERNAL, BRANCH, TAG
      :param build_pk: Build pk
+     :param commit: commit sha of the pull/merge request
      :param status: build status failed, pending, or success to be sent.
     """
 
     # Send status reports for only External (pull/merge request) Versions.
-    if version.type == EXTERNAL:
+    if version_type == EXTERNAL:
         # call the task that actually send the build status.
-        send_build_status.delay(build_pk, status)
+        send_build_status.delay(build_pk, commit, status)
diff --git a/readthedocs/rtd_tests/tests/test_api.py b/readthedocs/rtd_tests/tests/test_api.py
@@ -784,12 +784,13 @@ def setUp(self):
         self.github_payload = {
             'ref': 'master',
         }
+        self.commit = "ec26de721c3235aad62de7213c562f8c821"
         self.github_pull_request_payload = {
             "action": "opened",
             "number": 2,
             "pull_request": {
                 "head": {
-                    "sha": "ec26de721c3235aad62de7213c562f8c821"
+                    "sha": self.commit
                 }
             }
         }
@@ -932,7 +933,8 @@ def test_github_pull_request_opened_event(self, trigger_build, core_trigger_buil
         self.assertEqual(resp.data['project'], self.project.slug)
         self.assertEqual(resp.data['versions'], [external_version.verbose_name])
         core_trigger_build.assert_called_once_with(
-            force=True, project=self.project, version=external_version
+            force=True, project=self.project,
+            version=external_version, commit=self.commit
         )
         self.assertTrue(external_version)
 
@@ -963,7 +965,8 @@ def test_github_pull_request_reopened_event(self, trigger_build, core_trigger_bu
         self.assertEqual(resp.data['project'], self.project.slug)
         self.assertEqual(resp.data['versions'], [external_version.verbose_name])
         core_trigger_build.assert_called_once_with(
-            force=True, project=self.project, version=external_version
+            force=True, project=self.project,
+            version=external_version, commit=self.commit
         )
         self.assertTrue(external_version)
 
@@ -1007,7 +1010,8 @@ def test_github_pull_request_synchronize_event(self, trigger_build, core_trigger
         self.assertEqual(resp.data['project'], self.project.slug)
         self.assertEqual(resp.data['versions'], [external_version.verbose_name])
         core_trigger_build.assert_called_once_with(
-            force=True, project=self.project, version=external_version
+            force=True, project=self.project,
+            version=external_version, commit=self.commit
         )
         # `synchronize` webhook event updated the identifier (commit hash)
         self.assertNotEqual(prev_identifier, external_version.identifier)
diff --git a/readthedocs/rtd_tests/tests/test_celery.py b/readthedocs/rtd_tests/tests/test_celery.py
@@ -343,16 +343,22 @@ def test_send_build_status_task(self, send_build_status):
         external_build = get(
             Build, project=self.project, version=external_version
         )
-        tasks.send_build_status(external_build.id, BUILD_STATUS_SUCCESS)
+        tasks.send_build_status(
+            external_build.id, external_build.commit, BUILD_STATUS_SUCCESS
+        )
 
-        send_build_status.assert_called_once_with(external_build, BUILD_STATUS_SUCCESS)
+        send_build_status.assert_called_once_with(
+            external_build, external_build.commit, BUILD_STATUS_SUCCESS
+        )
 
     @patch('readthedocs.projects.tasks.GitHubService.send_build_status')
     def test_send_build_status_task_without_remote_repo(self, send_build_status):
         external_version = get(Version, project=self.project, type=EXTERNAL)
         external_build = get(
             Build, project=self.project, version=external_version
         )
-        tasks.send_build_status(external_build.id, BUILD_STATUS_SUCCESS)
+        tasks.send_build_status(
+            external_build.id, external_build.commit, BUILD_STATUS_SUCCESS
+        )
 
         send_build_status.assert_not_called()
diff --git a/readthedocs/rtd_tests/tests/test_config_integration.py b/readthedocs/rtd_tests/tests/test_config_integration.py
@@ -368,7 +368,10 @@ def get_update_docs_task(self):
             config=load_yaml_config(self.version),
             project=self.project,
             version=self.version,
-            build={'id': 99, 'state': BUILD_STATE_TRIGGERED}
+            build={
+                'id': 99,
+                'state': BUILD_STATE_TRIGGERED,
+            },
         )
         return update_docs
 
diff --git a/readthedocs/rtd_tests/tests/test_core_utils.py b/readthedocs/rtd_tests/tests/test_core_utils.py
diff --git a/readthedocs/rtd_tests/tests/test_oauth.py b/readthedocs/rtd_tests/tests/test_oauth.py
diff --git a/readthedocs/rtd_tests/tests/test_projects_tasks.py b/readthedocs/rtd_tests/tests/test_projects_tasks.py
