Skip to content
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.

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

Auto Build Documentation on Pull Request (PR Builder) #5828

Merged
merged 9 commits into from
Jun 21, 2019
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
35 changes: 34 additions & 1 deletion readthedocs/api/v2/views/integrations.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@
webhook_github,
webhook_gitlab,
)
from readthedocs.core.views.hooks import build_branches, sync_versions
from readthedocs.core.views.hooks import (
build_branches,
sync_versions,
get_or_create_external_version,
)
from readthedocs.integrations.models import HttpExchange, Integration
from readthedocs.projects.models import Project

Expand All @@ -29,6 +33,9 @@
GITHUB_EVENT_HEADER = 'HTTP_X_GITHUB_EVENT'
GITHUB_SIGNATURE_HEADER = 'HTTP_X_HUB_SIGNATURE'
GITHUB_PUSH = 'push'
GITHUB_PULL_REQUEST = 'pull_request'
GITHUB_PULL_REQUEST_OPEN = 'opened'
GITHUB_PULL_REQUEST_SYNC = 'synchronize'
GITHUB_CREATE = 'create'
GITHUB_DELETE = 'delete'
GITLAB_TOKEN_HEADER = 'HTTP_X_GITLAB_TOKEN'
Expand Down Expand Up @@ -110,6 +117,10 @@ def handle_webhook(self):
"""Handle webhook payload."""
raise NotImplementedError

def get_external_version_data(self):
"""Get External Version data from payload."""
raise NotImplementedError

def is_payload_valid(self):
"""Validates the webhook's payload using the integration's secret."""
return False
Expand Down Expand Up @@ -218,6 +229,13 @@ def get_data(self):
pass
return super().get_data()

def get_external_version_data(self):
"""Get Commit Sha and pull request number from payload"""
identifier = self.data['pull_request']['head']['sha']
verbose_name = str(self.data['number'])
Copy link
Member Author

Choose a reason for hiding this comment

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

here I'm considering PR_Number as verbose name.


return identifier, verbose_name

def is_payload_valid(self):
"""
GitHub use a HMAC hexdigest hash to sign the payload.
Expand Down Expand Up @@ -271,6 +289,21 @@ def handle_webhook(self):
raise ParseError('Parameter "ref" is required')
if event in (GITHUB_CREATE, GITHUB_DELETE):
return self.sync_versions(self.project)

if (
event == GITHUB_PULL_REQUEST and
self.data['action'] in [GITHUB_PULL_REQUEST_OPEN, GITHUB_PULL_REQUEST_SYNC]
Copy link
Member Author

Choose a reason for hiding this comment

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

This will handle both PR opened and synchronized (new commit in the PR) events.

Copy link
Member

Choose a reason for hiding this comment

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

Tested it locally and it worked great 👍

):
try:
identifier, verbose_name = self.get_external_version_data()
external_version = get_or_create_external_version(
self.project, identifier, verbose_name
)
return self.get_response_push(self.project, external_version.verbose_name)
Copy link
Member Author

Choose a reason for hiding this comment

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

To use the pre-written self.get_response_push() we can not pass a version instance so here I passed the verbose name but I think it will be much better if we pass the instance. to do this we need to create another function and not use self.get_response_push() we might use that function to send triggered status on the PR using the Github Status API. I would like to get you thought on this :)

Copy link
Member

Choose a reason for hiding this comment

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

Yea, we can use whatever makes sense. If it's doing something different than existing code, don't feel bad coming up with a new method.


except KeyError:
raise ParseError('Parameters "sha" and "number" are required')
Copy link
Member Author

Choose a reason for hiding this comment

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

The Error message can be changed


return None

def _normalize_ref(self, ref):
Expand Down
20 changes: 20 additions & 0 deletions readthedocs/core/views/hooks.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import logging

from readthedocs.builds.constants import EXTERNAL
from readthedocs.builds.models import Version
from readthedocs.core.utils import trigger_build
from readthedocs.projects.tasks import sync_repository_task

Expand Down Expand Up @@ -88,3 +90,21 @@ def sync_versions(project):
except Exception:
log.exception('Unknown sync versions exception')
return None


def get_or_create_external_version(project, identifier, verbose_name):
external_version = project.versions(manager=EXTERNAL).filter(verbose_name=verbose_name).first()
if external_version:
if external_version.identifier != identifier:
Copy link
Member Author

Choose a reason for hiding this comment

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

This will be updated when there is a new commit in the PR

Copy link
Member

Choose a reason for hiding this comment

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

This is probably better in a code comment, not a PR comment that will be lost :)

external_version.identifier = identifier
external_version.save()
else:
created_external_version = Version.objects.create(
project=project,
type=EXTERNAL,
identifier=identifier,
verbose_name=verbose_name,
active=True
)
return created_external_version
return external_version
2 changes: 2 additions & 0 deletions readthedocs/projects/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -333,3 +333,5 @@
'https://gitlab.com/{user}/{repo}/'
'{action}/{version}{docroot}{path}{source_suffix}'
)

GITHUB_GIT_PATTERN = 'pull/{id}/head:external-{id}'
2 changes: 1 addition & 1 deletion readthedocs/projects/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ def sync_repo(self):
}
)
version_repo = self.get_vcs_repo()
version_repo.update()
version_repo.update(version=self.version)
Copy link
Member Author

Choose a reason for hiding this comment

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

we need to pass the version here so that we can determine which PR should be fetched. there previous implementation fetched all the PR's all together regardless it was open or closed

Copy link
Member

Choose a reason for hiding this comment

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

Seems like a good and small change 👍

self.sync_versions(version_repo)
version_repo.checkout(self.version.identifier)
finally:
Expand Down
2 changes: 1 addition & 1 deletion readthedocs/vcs_support/backends/bzr.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ class Backend(BaseVCS):
supports_tags = True
fallback_branch = ''

def update(self):
def update(self, version=None): # pylint: disable=arguments-differ
super().update()
retcode = self.run('bzr', 'status', record=False)[0]
if retcode == 0:
Expand Down
22 changes: 19 additions & 3 deletions readthedocs/vcs_support/backends/git.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@
from django.core.exceptions import ValidationError
from git.exc import BadName, InvalidGitRepositoryError

from readthedocs.builds.constants import EXTERNAL
from readthedocs.config import ALL
from readthedocs.projects.constants import GITHUB_GIT_PATTERN
from readthedocs.projects.exceptions import RepositoryError
from readthedocs.projects.validators import validate_submodule_url
from readthedocs.vcs_support.base import BaseVCS, VCSVersion
Expand All @@ -25,6 +27,7 @@ class Backend(BaseVCS):

supports_tags = True
supports_branches = True
supports_external_branches = True
supports_submodules = True
fallback_branch = 'master' # default branch
repo_depth = 50
Expand All @@ -50,13 +53,20 @@ def _get_clone_url(self):
def set_remote_url(self, url):
return self.run('git', 'remote', 'set-url', 'origin', url)

def update(self):
def update(self, version=None): # pylint: disable=arguments-differ
"""Clone or update the repository."""
super().update()
if self.repo_exists():
self.set_remote_url(self.repo_url)
# A fetch is always required to get external versions properly
if version and version.type == EXTERNAL:
return self.fetch(version.verbose_name)
return self.fetch()
self.make_clean_working_dir()
# A fetch is always required to get external versions properly
if version and version.type == EXTERNAL:
self.clone()
return self.fetch(version.verbose_name)
return self.clone()

def repo_exists(self):
Expand Down Expand Up @@ -143,8 +153,14 @@ def use_shallow_clone(self):
from readthedocs.projects.models import Feature
return not self.project.has_feature(Feature.DONT_SHALLOW_CLONE)

def fetch(self):
cmd = ['git', 'fetch', '--tags', '--prune', '--prune-tags']
def fetch(self, verbose_name=None):
cmd = ['git', 'fetch', 'origin',
'--tags', '--prune', '--prune-tags']

if verbose_name and 'github.com' in self.repo_url:
cmd.append(
GITHUB_GIT_PATTERN.format(id=verbose_name)
)

if self.use_shallow_clone():
cmd.extend(['--depth', str(self.repo_depth)])
Expand Down
2 changes: 1 addition & 1 deletion readthedocs/vcs_support/backends/hg.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ class Backend(BaseVCS):
supports_branches = True
fallback_branch = 'default'

def update(self):
def update(self, version=None): # pylint: disable=arguments-differ
super().update()
retcode = self.run('hg', 'status', record=False)[0]
if retcode == 0:
Expand Down
2 changes: 1 addition & 1 deletion readthedocs/vcs_support/backends/svn.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ def __init__(self, *args, **kwargs):
else:
self.base_url = self.repo_url

def update(self):
def update(self, version=None): # pylint: disable=arguments-differ
super().update()
# For some reason `svn status` gives me retcode 0 in non-svn
# directories that's why I use `svn info` here.
Expand Down