From b1508c273a5fa36a2166778a158db0047d1d6f44 Mon Sep 17 00:00:00 2001 From: Benjamin Bach Date: Tue, 30 May 2023 15:18:37 +0200 Subject: [PATCH 1/5] Clarifications to state vs status --- readthedocs/builds/constants.py | 7 ++++++- readthedocs/oauth/services/github.py | 14 +++++++------- readthedocs/projects/tasks/builds.py | 7 ++++++- 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/readthedocs/builds/constants.py b/readthedocs/builds/constants.py index fd3c2a902c5..df1bd6bf7f3 100644 --- a/readthedocs/builds/constants.py +++ b/readthedocs/builds/constants.py @@ -3,6 +3,9 @@ from django.conf import settings from django.utils.translation import gettext_lazy as _ +# BUILD_STATE is our *INTERNAL* representation of build states. +# This is not to be confused with external representations of 'status' +# that are sent back to Git providers. BUILD_STATE_TRIGGERED = "triggered" BUILD_STATE_CLONING = "cloning" BUILD_STATE_INSTALLING = "installing" @@ -78,7 +81,9 @@ STABLE, ) -# General Build Statuses +# General build statuses, i.e. the status that is reported back to the +# user on a Git Provider. This not the same as BUILD_STATE which the internal +# representation. BUILD_STATUS_FAILURE = 'failed' BUILD_STATUS_PENDING = 'pending' BUILD_STATUS_SUCCESS = 'success' diff --git a/readthedocs/oauth/services/github.py b/readthedocs/oauth/services/github.py index 572f6e6fdba..d7efe70e1cc 100644 --- a/readthedocs/oauth/services/github.py +++ b/readthedocs/oauth/services/github.py @@ -446,8 +446,8 @@ def send_build_status(self, build, commit, status): project = build.project owner, repo = build_utils.get_github_username_repo(url=project.repo) - # select the correct state and description. - github_build_state = SELECT_BUILD_STATUS[status]["github"] + # select the correct status and description. + github_build_status = SELECT_BUILD_STATUS[status]["github"] description = SELECT_BUILD_STATUS[status]["description"] statuses_url = f"https://api.github.com/repos/{owner}/{repo}/statuses/{commit}" @@ -461,15 +461,15 @@ def send_build_status(self, build, commit, status): context = f'{settings.RTD_BUILD_STATUS_API_NAME}:{project.slug}' data = { - 'state': github_build_state, - 'target_url': target_url, - 'description': description, - 'context': context, + "state": github_build_status, + "target_url": target_url, + "description": description, + "context": context, } log.bind( project_slug=project.slug, - commit_status=github_build_state, + commit_status=github_build_status, user_username=self.user.username, statuses_url=statuses_url, ) diff --git a/readthedocs/projects/tasks/builds.py b/readthedocs/projects/tasks/builds.py index 55b00618351..1282efa2718 100644 --- a/readthedocs/projects/tasks/builds.py +++ b/readthedocs/projects/tasks/builds.py @@ -429,6 +429,8 @@ def on_failure(self, exc, task_id, args, kwargs, einfo): """ Celery handler to be executed when a task fails. + Updates build data, adds tasks to send build notifications. + .. note:: Since the task has failed, some attributes from the `self.data` @@ -520,7 +522,10 @@ def on_failure(self, exc, task_id, args, kwargs, einfo): ) # Update build object - self.data.build['success'] = False + if isinstance(exc, BuildUserSkip): + self.data.build["success"] = True + else: + self.data.build["success"] = False def get_valid_artifact_types(self): """ From a485f83195f901fab98dd9db091ddf2601d27861 Mon Sep 17 00:00:00 2001 From: Benjamin Bach Date: Wed, 31 May 2023 11:53:05 +0200 Subject: [PATCH 2/5] Use an absolute URL, including the "https://" for versions --- readthedocs/builds/models.py | 29 +++++++++++++++++++++------- readthedocs/oauth/services/github.py | 2 ++ readthedocs/projects/tasks/builds.py | 2 +- 3 files changed, 25 insertions(+), 8 deletions(-) diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index a2cc5bc1348..a8483bc9b6e 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -377,14 +377,29 @@ def commit_name(self): return self.identifier def get_absolute_url(self): - """Get absolute url to the docs of the version.""" + """ + Get the absolute URL to the docs of the version. + + If the version doesn't have a successfully uploaded build, then we return the project's + dashboard page. + + Because documentation projects can be hosted on separate domains, this function ALWAYS + returns with a full "http(s):///" prefix. + """ if not self.built and not self.uploaded: - return reverse( - 'project_version_detail', - kwargs={ - 'project_slug': self.project.slug, - 'version_slug': self.slug, - }, + # TODO: Stop assuming protocol based on settings.DEBUG + # (this pattern is also used in builds.tasks for sending emails) + protocol = "http" if settings.DEBUG else "https" + return "{}://{}{}".format( + protocol, + settings.PRODUCTION_DOMAIN, + reverse( + "project_version_detail", + kwargs={ + "project_slug": self.project.slug, + "version_slug": self.slug, + }, + ), ) external = self.type == EXTERNAL return self.project.get_docs_url( diff --git a/readthedocs/oauth/services/github.py b/readthedocs/oauth/services/github.py index d7efe70e1cc..69fb48a1a5d 100644 --- a/readthedocs/oauth/services/github.py +++ b/readthedocs/oauth/services/github.py @@ -472,6 +472,8 @@ def send_build_status(self, build, commit, status): commit_status=github_build_status, user_username=self.user.username, statuses_url=statuses_url, + target_url=target_url, + status=status, ) resp = None try: diff --git a/readthedocs/projects/tasks/builds.py b/readthedocs/projects/tasks/builds.py index 1282efa2718..30082fbb6b0 100644 --- a/readthedocs/projects/tasks/builds.py +++ b/readthedocs/projects/tasks/builds.py @@ -422,7 +422,7 @@ def before_start(self, task_id, args, kwargs): def _reset_build(self): # Reset build only if it has some commands already. if self.data.build.get("commands"): - log.info("Reseting build.") + log.info("Resetting build.") api_v2.build(self.data.build["id"]).reset.post() def on_failure(self, exc, task_id, args, kwargs, einfo): From 1b1d9fe39fa3de359fb13e9189c3f887b53dab7d Mon Sep 17 00:00:00 2001 From: Benjamin Bach Date: Wed, 31 May 2023 12:52:20 +0200 Subject: [PATCH 3/5] Update tests with new Version.get_absolute_url implementation --- readthedocs/rtd_tests/tests/test_api.py | 30 +++++++++++++++-------- readthedocs/rtd_tests/tests/test_oauth.py | 2 ++ 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_api.py b/readthedocs/rtd_tests/tests/test_api.py index 5d6b0703ce6..e425a16e302 100644 --- a/readthedocs/rtd_tests/tests/test_api.py +++ b/readthedocs/rtd_tests/tests/test_api.py @@ -6,6 +6,7 @@ import dateutil from allauth.socialaccount.models import SocialAccount from django.contrib.auth.models import User +from django.core.validators import URLValidator from django.http import QueryDict from django.test import TestCase, override_settings from django.urls import reverse @@ -299,12 +300,17 @@ def test_response_building(self): 'version_slug': version.slug, }, ) + build = resp.data - self.assertEqual(build['state'], 'cloning') - self.assertEqual(build['error'], '') - self.assertEqual(build['exit_code'], 0) - self.assertEqual(build['success'], True) - self.assertEqual(build['docs_url'], dashboard_url) + self.assertEqual(build["state"], "cloning") + self.assertEqual(build["error"], "") + self.assertEqual(build["exit_code"], 0) + self.assertEqual(build["success"], True) + self.assertTrue(build["docs_url"].endswith(dashboard_url)) + + # Validate received URL (fails with ValidationError if incorrect value) + url_validator = URLValidator(schemes=["https"]) + url_validator(build["docs_url"]) @override_settings(DOCROOT="/home/docs/checkouts/readthedocs.org/user_builds") def test_response_finished_and_success(self): @@ -390,11 +396,15 @@ def test_response_finished_and_fail(self): }, ) build = resp.data - self.assertEqual(build['state'], 'finished') - self.assertEqual(build['error'], '') - self.assertEqual(build['exit_code'], 1) - self.assertEqual(build['success'], False) - self.assertEqual(build['docs_url'], dashboard_url) + self.assertEqual(build["state"], "finished") + self.assertEqual(build["error"], "") + self.assertEqual(build["exit_code"], 1) + self.assertEqual(build["success"], False) + self.assertTrue(build["docs_url"].endswith(dashboard_url)) + + # Validate received URL (fails with ValidationError if incorrect value) + url_validator = URLValidator(schemes=["https"]) + url_validator(build["docs_url"]) def test_make_build_without_permission(self): """Ensure anonymous/non-staff users cannot write the build endpoint.""" diff --git a/readthedocs/rtd_tests/tests/test_oauth.py b/readthedocs/rtd_tests/tests/test_oauth.py index 8e0a171526f..5759756b723 100644 --- a/readthedocs/rtd_tests/tests/test_oauth.py +++ b/readthedocs/rtd_tests/tests/test_oauth.py @@ -350,6 +350,8 @@ def test_send_build_status_value_error(self, session, mock_logger): commit_status='success', user_username=self.user.username, statuses_url='https://api.github.com/repos/pypa/pip/statuses/1234', + target_url=mock.ANY, + status="success", ) mock_logger.exception.assert_called_with( 'GitHub commit status creation failed for project.', From 032950b3f56b45cec5e7aa569d3d645b12bf67db Mon Sep 17 00:00:00 2001 From: Benjamin Bach Date: Wed, 31 May 2023 15:29:02 +0200 Subject: [PATCH 4/5] Revert a change: Always set `self.data.build["success"] = False` in on_failure --- readthedocs/projects/tasks/builds.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/readthedocs/projects/tasks/builds.py b/readthedocs/projects/tasks/builds.py index 30082fbb6b0..bbe3fd2d034 100644 --- a/readthedocs/projects/tasks/builds.py +++ b/readthedocs/projects/tasks/builds.py @@ -522,10 +522,7 @@ def on_failure(self, exc, task_id, args, kwargs, einfo): ) # Update build object - if isinstance(exc, BuildUserSkip): - self.data.build["success"] = True - else: - self.data.build["success"] = False + self.data.build["success"] = False def get_valid_artifact_types(self): """ From daad79e57b31a3b8d4ced2965bcf5f88dbdc6745 Mon Sep 17 00:00:00 2001 From: Benjamin Bach Date: Wed, 31 May 2023 15:31:21 +0200 Subject: [PATCH 5/5] Don't use URLValidator in tests --- readthedocs/rtd_tests/tests/test_api.py | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_api.py b/readthedocs/rtd_tests/tests/test_api.py index e425a16e302..92906d17550 100644 --- a/readthedocs/rtd_tests/tests/test_api.py +++ b/readthedocs/rtd_tests/tests/test_api.py @@ -6,7 +6,6 @@ import dateutil from allauth.socialaccount.models import SocialAccount from django.contrib.auth.models import User -from django.core.validators import URLValidator from django.http import QueryDict from django.test import TestCase, override_settings from django.urls import reverse @@ -307,10 +306,7 @@ def test_response_building(self): self.assertEqual(build["exit_code"], 0) self.assertEqual(build["success"], True) self.assertTrue(build["docs_url"].endswith(dashboard_url)) - - # Validate received URL (fails with ValidationError if incorrect value) - url_validator = URLValidator(schemes=["https"]) - url_validator(build["docs_url"]) + self.assertTrue(build["docs_url"].startswith("https://")) @override_settings(DOCROOT="/home/docs/checkouts/readthedocs.org/user_builds") def test_response_finished_and_success(self): @@ -401,10 +397,7 @@ def test_response_finished_and_fail(self): self.assertEqual(build["exit_code"], 1) self.assertEqual(build["success"], False) self.assertTrue(build["docs_url"].endswith(dashboard_url)) - - # Validate received URL (fails with ValidationError if incorrect value) - url_validator = URLValidator(schemes=["https"]) - url_validator(build["docs_url"]) + self.assertTrue(build["docs_url"].startswith("https://")) def test_make_build_without_permission(self): """Ensure anonymous/non-staff users cannot write the build endpoint."""