From 6a14ffac5198da0d22119e2ba9d5673197e9acf3 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 13 Aug 2020 15:25:58 -0500 Subject: [PATCH 1/3] External providers: better logging for GitLab Ref https://github.com/readthedocs/readthedocs.org/issues/7370 --- readthedocs/oauth/services/gitlab.py | 19 +++++++------------ readthedocs/projects/tasks.py | 1 - 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/readthedocs/oauth/services/gitlab.py b/readthedocs/oauth/services/gitlab.py index df7ee2a7ed5..27b4bec2888 100644 --- a/readthedocs/oauth/services/gitlab.py +++ b/readthedocs/oauth/services/gitlab.py @@ -3,29 +3,24 @@ import json import logging import re +from urllib.parse import urljoin, urlparse from allauth.socialaccount.providers.gitlab.views import GitLabOAuth2Adapter from django.conf import settings from django.urls import reverse from requests.exceptions import RequestException +from readthedocs.builds import utils as build_utils from readthedocs.builds.constants import ( BUILD_STATUS_SUCCESS, SELECT_BUILD_STATUS, ) -from readthedocs.builds import utils as build_utils from readthedocs.integrations.models import Integration from readthedocs.projects.models import Project from ..models import RemoteOrganization, RemoteRepository from .base import Service, SyncServiceError - -try: - from urlparse import urljoin, urlparse -except ImportError: - from urllib.parse import urljoin, urlparse # noqa - log = logging.getLogger(__name__) @@ -539,16 +534,16 @@ def send_build_status(self, build, commit, state): if resp.status_code == 201: log.info( "GitLab commit status created for project: %s, commit status: %s", - project, + project.slug, gitlab_build_state, ) return True if resp.status_code in [401, 403, 404]: log.info( - 'GitLab project does not exist or user does not have ' - 'permissions: project=%s', - project, + 'GitLab project does not exist or user does not have permissions: ' + 'project=%s user=%s status_code=%s', + project.slug, self.user.username, resp.status_code, ) return False @@ -558,7 +553,7 @@ def send_build_status(self, build, commit, state): except (RequestException, ValueError): log.exception( 'GitLab commit status creation failed for project: %s', - project, + project.slug, ) # Response data should always be JSON, still try to log if not # though diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 4df043c0096..acd7b937768 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -1941,7 +1941,6 @@ def send_build_status(build_pk, commit, status): user_accounts = service_class.for_user(user) # Try to loop through users all social accounts to send a successful request for account in user_accounts: - # Currently we only support GitHub Status API if account.provider_name == provider_name: success = account.send_build_status(build, commit, status) if success: From bf95ba5a3ef2f9f4ca72f6b4bab3c69180966574 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 13 Aug 2020 15:40:54 -0500 Subject: [PATCH 2/3] Log in github and gitlab --- readthedocs/oauth/services/github.py | 14 ++++++-------- readthedocs/oauth/services/gitlab.py | 7 ++++--- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/readthedocs/oauth/services/github.py b/readthedocs/oauth/services/github.py index cc18aa6df7f..21bbae86e76 100644 --- a/readthedocs/oauth/services/github.py +++ b/readthedocs/oauth/services/github.py @@ -6,9 +6,8 @@ from allauth.socialaccount.models import SocialToken from allauth.socialaccount.providers.github.views import GitHubOAuth2Adapter - -from django.db.models import Q from django.conf import settings +from django.db.models import Q from django.urls import reverse from requests.exceptions import RequestException @@ -23,7 +22,6 @@ from ..models import RemoteOrganization, RemoteRepository from .base import Service, SyncServiceError - log = logging.getLogger(__name__) @@ -465,7 +463,7 @@ def send_build_status(self, build, commit, state): if resp.status_code == 201: log.info( "GitHub commit status created for project: %s, commit status: %s", - project, + project.slug, github_build_state, ) return True @@ -474,8 +472,8 @@ def send_build_status(self, build, commit, state): log.info( 'GitHub project does not exist or user does not have ' 'permissions: project=%s, user=%s, status=%s, url=%s', - project, - self.user, + project.slug, + self.user.username, resp.status_code, statuses_url, ) @@ -483,7 +481,7 @@ def send_build_status(self, build, commit, state): log.warning( 'Unknown GitHub status API response: project=%s, user=%s, status_code=%s', - project, + project.slug, self.user, resp.status_code ) @@ -493,7 +491,7 @@ def send_build_status(self, build, commit, state): except (RequestException, ValueError): log.exception( 'GitHub commit status creation failed for project: %s', - project, + project.slug, ) # Response data should always be JSON, still try to log if not # though diff --git a/readthedocs/oauth/services/gitlab.py b/readthedocs/oauth/services/gitlab.py index 27b4bec2888..5f3dba7368b 100644 --- a/readthedocs/oauth/services/gitlab.py +++ b/readthedocs/oauth/services/gitlab.py @@ -525,8 +525,9 @@ def send_build_status(self, build, commit, state): url = self.adapter.provider_base_url try: + statuses_url = f'{url}/api/v4/projects/{repo_id}/statuses/{commit}' resp = session.post( - f'{url}/api/v4/projects/{repo_id}/statuses/{commit}', + statuses_url, data=json.dumps(data), headers={'content-type': 'application/json'}, ) @@ -542,8 +543,8 @@ def send_build_status(self, build, commit, state): if resp.status_code in [401, 403, 404]: log.info( 'GitLab project does not exist or user does not have permissions: ' - 'project=%s user=%s status_code=%s', - project.slug, self.user.username, resp.status_code, + 'project=%s, user=%s, status=%s, url=%s', + project.slug, self.user.username, resp.status_code, statuses_url, ) return False From d0c455be91bf6f9dcb4fd31582cbfd9b845fad3c Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 13 Aug 2020 16:58:36 -0500 Subject: [PATCH 3/3] Update tests --- readthedocs/rtd_tests/tests/test_oauth.py | 34 ++++++++++++----------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_oauth.py b/readthedocs/rtd_tests/tests/test_oauth.py index e664a57f75a..24b197ee168 100644 --- a/readthedocs/rtd_tests/tests/test_oauth.py +++ b/readthedocs/rtd_tests/tests/test_oauth.py @@ -1,19 +1,18 @@ -# -*- coding: utf-8 -*- from unittest import mock + from django.conf import settings from django.contrib.auth.models import User from django.test import TestCase from django.test.utils import override_settings from django.urls import reverse - from django_dynamic_fixture import get -from readthedocs.builds.constants import EXTERNAL, BUILD_STATUS_SUCCESS -from readthedocs.builds.models import Version, Build +from readthedocs.builds.constants import BUILD_STATUS_SUCCESS, EXTERNAL +from readthedocs.builds.models import Build, Version from readthedocs.integrations.models import ( + BitbucketWebhook, GitHubWebhook, GitLabWebhook, - BitbucketWebhook ) from readthedocs.oauth.models import RemoteOrganization, RemoteRepository from readthedocs.oauth.services import ( @@ -184,7 +183,7 @@ def test_send_build_status_successful(self, session, mock_logger): self.assertTrue(success) mock_logger.info.assert_called_with( "GitHub commit status created for project: %s, commit status: %s", - self.project, + self.project.slug, BUILD_STATUS_SUCCESS ) @@ -202,8 +201,8 @@ def test_send_build_status_404_error(self, session, mock_logger): mock_logger.info.assert_called_with( 'GitHub project does not exist or user does not have ' 'permissions: project=%s, user=%s, status=%s, url=%s', - self.project, - self.user, + self.project.slug, + self.user.username, 404, 'https://api.github.com/repos/pypa/pip/statuses/1234' ) @@ -219,7 +218,7 @@ def test_send_build_status_value_error(self, session, mock_logger): self.assertFalse(success) mock_logger.exception.assert_called_with( 'GitHub commit status creation failed for project: %s', - self.project, + self.project.slug, ) @override_settings(DEFAULT_PRIVACY_LEVEL='private') @@ -927,7 +926,7 @@ def setUp(self): self.service = GitLabService(user=self.user, account=None) self.external_version = get(Version, project=self.project, type=EXTERNAL) self.external_build = get( - Build, project=self.project, version=self.external_version + Build, project=self.project, version=self.external_version, commit=1234, ) self.integration = get( GitLabWebhook, @@ -1031,7 +1030,7 @@ def test_send_build_status_successful(self, repo_id, session, mock_logger): self.assertTrue(success) mock_logger.info.assert_called_with( "GitLab commit status created for project: %s, commit status: %s", - self.project, + self.project.slug, BUILD_STATUS_SUCCESS ) @@ -1040,7 +1039,7 @@ def test_send_build_status_successful(self, repo_id, session, mock_logger): @mock.patch('readthedocs.oauth.services.gitlab.GitLabService._get_repo_id') def test_send_build_status_404_error(self, repo_id, session, mock_logger): session().post.return_value.status_code = 404 - repo_id().return_value = '9999' + repo_id.return_value = '9999' success = self.service.send_build_status( self.external_build, @@ -1050,9 +1049,12 @@ def test_send_build_status_404_error(self, repo_id, session, mock_logger): self.assertFalse(success) mock_logger.info.assert_called_with( - 'GitLab project does not exist or user does not have ' - 'permissions: project=%s', - self.project + 'GitLab project does not exist or user does not have permissions: ' + 'project=%s, user=%s, status=%s, url=%s', + self.project.slug, + self.user.username, + 404, + 'https://gitlab.com/api/v4/projects/9999/statuses/1234', ) @mock.patch('readthedocs.oauth.services.gitlab.log') @@ -1069,7 +1071,7 @@ def test_send_build_status_value_error(self, repo_id, session, mock_logger): self.assertFalse(success) mock_logger.exception.assert_called_with( 'GitLab commit status creation failed for project: %s', - self.project, + self.project.slug, ) @mock.patch('readthedocs.oauth.services.gitlab.log')