Skip to content

Commit 2c963f1

Browse files
authored
Git service: depend on the project instead of users (#11983)
With #11942, a GH app is used to interact with the GH API, a GH app is not directly tied to a user, so this assumption is not valid anymore. For most of the operations (except for syncing repos), we don't need to know who is the user that is performing the action, we just need to know the project that is being affected. - The base provider doesn't assume that we are always using an oauth token, that's an implementation detail of each provider. We also no longer directly depend on allauth from the base provider. - Every provider implements the operations, no more checks for specific providers (e.g checking for BB when sending a build status).
1 parent 8efd774 commit 2c963f1

File tree

16 files changed

+275
-273
lines changed

16 files changed

+275
-273
lines changed

readthedocs/builds/tasks.py

Lines changed: 33 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -30,12 +30,10 @@
3030
)
3131
from readthedocs.builds.models import Build, Version
3232
from readthedocs.builds.utils import memcache_lock
33-
from readthedocs.core.permissions import AdminPermission
3433
from readthedocs.core.utils import send_email, trigger_build
3534
from readthedocs.integrations.models import HttpExchange
3635
from readthedocs.notifications.models import Notification
3736
from readthedocs.oauth.notifications import MESSAGE_OAUTH_BUILD_STATUS_FAILURE
38-
from readthedocs.projects.constants import GITHUB_BRAND, GITLAB_BRAND
3937
from readthedocs.projects.models import Project, WebHookEvent
4038
from readthedocs.storage import build_commands_storage
4139
from readthedocs.worker import app
@@ -385,24 +383,16 @@ def sync_versions_task(project_pk, tags_data, branches_data, **kwargs):
385383
@app.task(max_retries=3, default_retry_delay=60, queue="web")
386384
def send_build_status(build_pk, commit, status):
387385
"""
388-
Send Build Status to Git Status API for project external versions.
389-
390-
It tries using these services' account in order:
391-
392-
1. user's account that imported the project
393-
2. each user's account from the project's maintainers
386+
Send build status to GitHub/GitLab for a given build/commit.
394387
395388
:param build_pk: Build primary key
396389
:param commit: commit sha of the pull/merge request
397390
:param status: build status failed, pending, or success to be sent.
398391
"""
399-
# TODO: Send build status for Bitbucket.
400392
build = Build.objects.filter(pk=build_pk).first()
401393
if not build:
402394
return
403395

404-
provider_name = build.project.git_provider_name
405-
406396
log.bind(
407397
build_id=build.pk,
408398
project_slug=build.project.slug,
@@ -412,76 +402,42 @@ def send_build_status(build_pk, commit, status):
412402

413403
log.debug("Sending build status.")
414404

415-
if provider_name in [GITHUB_BRAND, GITLAB_BRAND]:
416-
# get the service class for the project e.g: GitHubService.
417-
service_class = build.project.git_service_class()
418-
users = AdminPermission.admins(build.project)
419-
420-
if build.project.remote_repository:
421-
remote_repository = build.project.remote_repository
422-
remote_repository_relations = (
423-
remote_repository.remote_repository_relations.filter(
424-
account__isnull=False,
425-
# Use ``user_in=`` instead of ``user__projects=`` here
426-
# because User's are not related to Project's directly in
427-
# Read the Docs for Business
428-
user__in=AdminPermission.members(build.project),
429-
)
430-
.select_related("account", "user")
431-
.only("user", "account")
432-
)
405+
# Get the service class for the project e.g: GitHubService.
406+
# We fallback to guess the service from the repo,
407+
# in the future we should only consider projects that have a remote repository.
408+
service_class = build.project.get_git_service_class(fallback_to_clone_url=True)
409+
if not service_class:
410+
log.info("Project isn't connected to a Git service, not sending build status.")
411+
return False
433412

434-
# Try using any of the users' maintainer accounts
435-
# Try to loop through all remote repository relations for the projects users
436-
for relation in remote_repository_relations:
437-
service = service_class(relation.user, relation.account)
438-
# Send status report using the API.
439-
success = service.send_build_status(
440-
build,
441-
commit,
442-
status,
443-
)
413+
if not service_class.supports_build_status:
414+
log.info("Git service doesn't support build status.")
415+
return False
444416

445-
if success:
446-
log.debug(
447-
"Build status report sent correctly.",
448-
user_username=relation.user.username,
449-
)
450-
return True
451-
else:
452-
log.warning("Project does not have a RemoteRepository.")
453-
# Try to send build status for projects with no RemoteRepository
454-
for user in users:
455-
services = service_class.for_user(user)
456-
# Try to loop through services for users all social accounts
457-
# to send successful build status
458-
for service in services:
459-
success = service.send_build_status(
460-
build,
461-
commit,
462-
status,
463-
)
464-
if success:
465-
log.debug(
466-
"Build status report sent correctly using an user account.",
467-
user_username=user.username,
468-
)
469-
return True
470-
471-
# NOTE: this notifications was attached to every user.
472-
# Now, I'm attaching it to the project itself since it's a problem at project level.
473-
Notification.objects.add(
474-
message_id=MESSAGE_OAUTH_BUILD_STATUS_FAILURE,
475-
attached_to=build.project,
476-
format_values={
477-
"provider_name": provider_name,
478-
"url_connect_account": reverse("socialaccount_connections"),
479-
},
480-
dismissable=True,
417+
for service in service_class.for_project(build.project):
418+
success = service.send_build_status(
419+
build,
420+
commit,
421+
status,
481422
)
423+
if success:
424+
log.debug("Build status report sent correctly.")
425+
return True
426+
427+
Notification.objects.add(
428+
message_id=MESSAGE_OAUTH_BUILD_STATUS_FAILURE,
429+
attached_to=build.project,
430+
format_values={
431+
"provider_name": service_class.provider_name,
432+
"url_connect_account": reverse("socialaccount_connections"),
433+
},
434+
dismissable=True,
435+
)
482436

483-
log.info("No social account or repository permission available.")
484-
return False
437+
log.info(
438+
"No social account or repository permission available, no build status sent."
439+
)
440+
return False
485441

486442

487443
@app.task(queue="web")

readthedocs/oauth/models.py

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
"""OAuth service models."""
2-
2+
import structlog
33
from allauth.socialaccount.models import SocialAccount
44
from django.contrib.auth.models import User
55
from django.core.validators import URLValidator
@@ -14,6 +14,8 @@
1414
from .constants import VCS_PROVIDER_CHOICES
1515
from .querysets import RemoteOrganizationQuerySet, RemoteRepositoryQuerySet
1616

17+
log = structlog.get_logger(__name__)
18+
1719

1820
class RemoteOrganization(TimeStampedModel):
1921

@@ -224,6 +226,19 @@ def get_remote_repository_relation(self, user, social_account):
224226
)
225227
return remote_repository_relation
226228

229+
def get_service_class(self):
230+
from readthedocs.oauth.services import registry
231+
232+
for service_cls in registry:
233+
if service_cls.vcs_provider_slug == self.vcs_provider:
234+
return service_cls
235+
236+
# NOTE: this should never happen, but we log it just in case
237+
log.exception(
238+
"Service not found for the VCS provider", vcs_provider=self.vcs_provider
239+
)
240+
return None
241+
227242

228243
class RemoteRepositoryRelation(TimeStampedModel):
229244
remote_repository = models.ForeignKey(

0 commit comments

Comments
 (0)