Skip to content

External providers: better logging for GitLab #7385

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 13, 2020
Merged

External providers: better logging for GitLab #7385

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6a14ffa
External providers: better logging for GitLab
stsewd Aug 13, 2020
bf95ba5
Log in github and gitlab
stsewd Aug 13, 2020
d0c455b
Update tests
stsewd Aug 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions readthedocs/oauth/services/github.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -23,7 +22,6 @@
from ..models import RemoteOrganization, RemoteRepository
from .base import Service, SyncServiceError


log = logging.getLogger(__name__)


Expand Down Expand Up @@ -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
Expand All @@ -474,16 +472,16 @@ 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,
)
return False

log.warning(
'Unknown GitHub status API response: project=%s, user=%s, status_code=%s',
project,
project.slug,
self.user,
resp.status_code
)
Expand All @@ -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
Expand Down
22 changes: 9 additions & 13 deletions readthedocs/oauth/services/gitlab.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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__)


Expand Down Expand Up @@ -530,25 +525,26 @@ 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'},
)

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: '
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably make sure our logging is consistent here across all providers. I've definitely wanted better logging here in the past, so 👍 on improvements.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ha, looks like github was already logging this information, and more, I have updated this pr to match both

'project=%s, user=%s, status=%s, url=%s',
project.slug, self.user.username, resp.status_code, statuses_url,
)
return False

Expand All @@ -558,7 +554,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
Expand Down
1 change: 0 additions & 1 deletion readthedocs/projects/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
34 changes: 18 additions & 16 deletions readthedocs/rtd_tests/tests/test_oauth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 (
Expand Down Expand Up @@ -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
)

Expand All @@ -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'
)
Expand All @@ -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')
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
)

Expand All @@ -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,
Expand All @@ -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')
Expand All @@ -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')
Expand Down