From e10a45457e77b317ee9eab43d69be6d810f7004a Mon Sep 17 00:00:00 2001 From: Eric Holscher Date: Tue, 28 Feb 2023 13:42:08 -0800 Subject: [PATCH 1/4] Fix checking of PR status This was previously checking the status of the build object, but I think we want to check the status that is passed in, and only link to the built docs when the build was successful. --- readthedocs/builds/tasks.py | 8 ++++---- readthedocs/oauth/services/base.py | 6 +++--- readthedocs/oauth/services/github.py | 16 ++++++++-------- readthedocs/oauth/services/gitlab.py | 16 ++++++++-------- readthedocs/projects/tasks/utils.py | 2 +- 5 files changed, 24 insertions(+), 24 deletions(-) diff --git a/readthedocs/builds/tasks.py b/readthedocs/builds/tasks.py index f1abb11f202..d52052ff3c2 100644 --- a/readthedocs/builds/tasks.py +++ b/readthedocs/builds/tasks.py @@ -433,7 +433,7 @@ def send_build_status(build_pk, commit, status): success = service.send_build_status( build=build, commit=commit, - state=status, + status=status, ) if success: @@ -451,9 +451,9 @@ def send_build_status(build_pk, commit, status): # to send successful build status for service in services: success = service.send_build_status( - build, - commit, - status, + build=build, + commit=commit, + status=status, ) if success: log.debug( diff --git a/readthedocs/oauth/services/base.py b/readthedocs/oauth/services/base.py index 812aef60cae..1539fbccb01 100644 --- a/readthedocs/oauth/services/base.py +++ b/readthedocs/oauth/services/base.py @@ -293,7 +293,7 @@ def update_webhook(self, project, integration): """ raise NotImplementedError - def send_build_status(self, build, commit, state): + def send_build_status(self, build, commit, status): """ Create commit status for project. @@ -301,8 +301,8 @@ def send_build_status(self, build, commit, state): :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 + :param status: build state failure, pending, or success. + :type status: str :returns: boolean based on commit status creation was successful or not. :rtype: Bool """ diff --git a/readthedocs/oauth/services/github.py b/readthedocs/oauth/services/github.py index ed44f7cde8c..bb69b357e74 100644 --- a/readthedocs/oauth/services/github.py +++ b/readthedocs/oauth/services/github.py @@ -13,7 +13,7 @@ from readthedocs.api.v2.client import api from readthedocs.builds import utils as build_utils -from readthedocs.builds.constants import BUILD_FINAL_STATES, SELECT_BUILD_STATUS +from readthedocs.builds.constants import BUILD_STATUS_SUCCESS, SELECT_BUILD_STATUS from readthedocs.core.permissions import AdminPermission from readthedocs.integrations.models import Integration @@ -432,14 +432,14 @@ def update_webhook(self, project, integration): integration.remove_secret() return (False, resp) - def send_build_status(self, build, commit, state): + def send_build_status(self, build, commit, status): """ 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 status: build state failure, pending, or success. + :type status: str :param commit: commit sha of the pull request :type commit: str :returns: boolean based on commit status creation was successful or not. @@ -450,11 +450,11 @@ def send_build_status(self, build, commit, state): owner, repo = build_utils.get_github_username_repo(url=project.repo) # select the correct state and description. - github_build_state = SELECT_BUILD_STATUS[state]['github'] - description = SELECT_BUILD_STATUS[state]['description'] - statuses_url = f'https://api.github.com/repos/{owner}/{repo}/statuses/{commit}' + github_build_state = SELECT_BUILD_STATUS[status]["github"] + description = SELECT_BUILD_STATUS[status]["description"] + statuses_url = f"https://api.github.com/repos/{owner}/{repo}/statuses/{commit}" - if build.state in BUILD_FINAL_STATES and build.success: + if status == BUILD_STATUS_SUCCESS: # Link to the documentation for this version target_url = build.version.get_absolute_url() else: diff --git a/readthedocs/oauth/services/gitlab.py b/readthedocs/oauth/services/gitlab.py index 95d1f6247c5..d5cbe91f783 100644 --- a/readthedocs/oauth/services/gitlab.py +++ b/readthedocs/oauth/services/gitlab.py @@ -12,7 +12,7 @@ from requests.exceptions import RequestException from readthedocs.builds import utils as build_utils -from readthedocs.builds.constants import BUILD_FINAL_STATES, SELECT_BUILD_STATUS +from readthedocs.builds.constants import BUILD_STATUS_SUCCESS, SELECT_BUILD_STATUS from readthedocs.integrations.models import Integration from ..constants import GITLAB @@ -519,14 +519,14 @@ def update_webhook(self, project, integration): integration.remove_secret() return (False, resp) - def send_build_status(self, build, commit, state): + def send_build_status(self, build, commit, status): """ Create GitLab 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 status: build status failure, pending, or success. + :type status: str :param commit: commit sha of the pull request :type commit: str :returns: boolean based on commit status creation was successful or not. @@ -541,11 +541,11 @@ def send_build_status(self, build, commit, state): if repo_id is None: return (False, resp) - # select the correct state and description. - gitlab_build_state = SELECT_BUILD_STATUS[state]['gitlab'] - description = SELECT_BUILD_STATUS[state]['description'] + # select the correct status and description. + gitlab_build_state = SELECT_BUILD_STATUS[status]["gitlab"] + description = SELECT_BUILD_STATUS[status]["description"] - if build.state in BUILD_FINAL_STATES and build.success: + if status == BUILD_STATUS_SUCCESS: # Link to the documentation for this version target_url = build.version.get_absolute_url() else: diff --git a/readthedocs/projects/tasks/utils.py b/readthedocs/projects/tasks/utils.py index da5a16c667f..ba521e5b10a 100644 --- a/readthedocs/projects/tasks/utils.py +++ b/readthedocs/projects/tasks/utils.py @@ -151,7 +151,7 @@ def send_external_build_status(version_type, build_pk, commit, status): # Send status reports for only External (pull/merge request) Versions. if version_type == EXTERNAL: # call the task that actually send the build status. - send_build_status.delay(build_pk, commit, status) + send_build_status.delay(build=build_pk, commit=commit, status=status) class BuildRequest(Request): From 75b2a0dfbc2f30941fb0a71e3f615e3b522bbf5d Mon Sep 17 00:00:00 2001 From: Benjamin Bach Date: Sat, 22 Apr 2023 20:56:01 +0200 Subject: [PATCH 2/4] use positional arguments, doesn't seem like celery can use them as kwargs --- readthedocs/projects/tasks/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/projects/tasks/utils.py b/readthedocs/projects/tasks/utils.py index ba521e5b10a..da5a16c667f 100644 --- a/readthedocs/projects/tasks/utils.py +++ b/readthedocs/projects/tasks/utils.py @@ -151,7 +151,7 @@ def send_external_build_status(version_type, build_pk, commit, status): # Send status reports for only External (pull/merge request) Versions. if version_type == EXTERNAL: # call the task that actually send the build status. - send_build_status.delay(build=build_pk, commit=commit, status=status) + send_build_status.delay(build_pk, commit, status) class BuildRequest(Request): From 309cd3fbe4f2b4e15968fbbc4f2553490aad9e09 Mon Sep 17 00:00:00 2001 From: Benjamin Bach Date: Sat, 22 Apr 2023 21:59:49 +0200 Subject: [PATCH 3/4] Revert remaining kwargs calls to positional args --- readthedocs/builds/tasks.py | 12 ++++++------ readthedocs/rtd_tests/tests/test_celery.py | 12 ++++++------ 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/readthedocs/builds/tasks.py b/readthedocs/builds/tasks.py index d52052ff3c2..e66cacdf388 100644 --- a/readthedocs/builds/tasks.py +++ b/readthedocs/builds/tasks.py @@ -431,9 +431,9 @@ def send_build_status(build_pk, commit, status): service = service_class(relation.user, relation.account) # Send status report using the API. success = service.send_build_status( - build=build, - commit=commit, - status=status, + build, + commit, + status, ) if success: @@ -451,9 +451,9 @@ def send_build_status(build_pk, commit, status): # to send successful build status for service in services: success = service.send_build_status( - build=build, - commit=commit, - status=status, + build, + commit, + status, ) if success: log.debug( diff --git a/readthedocs/rtd_tests/tests/test_celery.py b/readthedocs/rtd_tests/tests/test_celery.py index cde5d76ee5e..b3fd0384de8 100644 --- a/readthedocs/rtd_tests/tests/test_celery.py +++ b/readthedocs/rtd_tests/tests/test_celery.py @@ -98,9 +98,9 @@ def test_send_build_status_with_remote_repo_github(self, send_build_status): ) send_build_status.assert_called_once_with( - build=external_build, - commit=external_build.commit, - state=BUILD_STATUS_SUCCESS, + external_build, + external_build.commit, + BUILD_STATUS_SUCCESS, ) self.assertEqual(Message.objects.filter(user=self.eric).count(), 0) @@ -165,9 +165,9 @@ def test_send_build_status_with_remote_repo_gitlab(self, send_build_status): ) send_build_status.assert_called_once_with( - build=external_build, - commit=external_build.commit, - state=BUILD_STATUS_SUCCESS, + external_build, + external_build.commit, + BUILD_STATUS_SUCCESS, ) self.assertEqual(Message.objects.filter(user=self.eric).count(), 0) From ef5ea62ffff4a4a723fe67a46cc1a026c7e1ca2c Mon Sep 17 00:00:00 2001 From: Benjamin Bach Date: Mon, 1 May 2023 23:52:31 +0200 Subject: [PATCH 4/4] hello darker my old friend --- readthedocs/oauth/services/github.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/readthedocs/oauth/services/github.py b/readthedocs/oauth/services/github.py index 151e45ed791..572f6e6fdba 100644 --- a/readthedocs/oauth/services/github.py +++ b/readthedocs/oauth/services/github.py @@ -11,8 +11,7 @@ from requests.exceptions import RequestException from readthedocs.builds import utils as build_utils -from readthedocs.builds.constants import BUILD_STATUS_SUCCESS, BUILD_FINAL_STATES, SELECT_BUILD_STATUS -from readthedocs.core.permissions import AdminPermission +from readthedocs.builds.constants import BUILD_STATUS_SUCCESS, SELECT_BUILD_STATUS from readthedocs.integrations.models import Integration from ..constants import GITHUB