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

Pull Request Builder Design Doc #5705

Merged
merged 15 commits into from
Jun 12, 2019
103 changes: 103 additions & 0 deletions docs/design/pr-builder.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
Design of Pull Request Builder
==============================

Background
----------

This will focus on automatically building documentation for Pull Requests
on Read the Docs projects. This is one of the most requested feature of
Read the Docs. This document will serve as a design document
for discussing how to implement this features.

Scope
-----

saadmk11 marked this conversation as resolved.
Show resolved Hide resolved
- Receiving ``pull_request`` webhook event from Github
- Fetching data from pull requests.
- Making Pull Requests work like temporary ``Version``
- Creating PR Versions when a pull request is opened and Triggering a build
- Setting PR version Life time
- Storing PR Version build Data.
- Deleting PR version and the build data
- Excluding PR Versions from Search Engines
- Serving PR Docs
stsewd marked this conversation as resolved.
Show resolved Hide resolved

Fetching Data from Pull Requests
--------------------------------

We already get Pull requests event from Github webhooks.
We can utilize that to fetch data from pull requests.
when a ``pull_request`` event is triggered we can fetch
the data of that pull request.
We can fetch the pull request by doing something similar to travis-ci.
ie: ``git fetch origin +refs/pull/<pr_number>/merge:``

Modeling Pull Requests as a type of version
-------------------------------------------

Pull requests can be Treated as a Type of Temporary ``Version``
We might consider adding a Boolean Field or ``VERSION_TYPES`` to the ``Version`` model.

- If we go with Boolean Field we can add something like ``is_pull_request`` Field.
- If we go with ``VERSION_TYPES`` we can add something like ``pull_request`` alongside Tag and Branche.

We also have to update the current ``Version`` model ``QuerySet`` to exclude the PR versions.
We have to add ``QuerySet`` for PR versions also.
stsewd marked this conversation as resolved.
Show resolved Hide resolved

Creating Versions for Pull Requests
-----------------------------------

If the Github webhook event is ``pull_request`` and action is ``opened``
this means a pull request was opened in the projects repository. We can create a ``Version``
from the Payload data and trigger a initial build for the version. A version will be created
whenever RTD receives a event like this.

Triggering Build for New Commits in a Pull Request
--------------------------------------------------

We might want to trigger a new build for the PR version
if there is a new commit on the PR.


Storing Pull Request Docs
-------------------------

We need to think about how and where to store data after a PR Version build is finished.
We can store the data in a blob storage.

Deleting a PR Version
---------------------

We can delete a PR version when:

- A pull request is ``closed``. Github Webhook event (Action: ``closed``, Merged: ``False``)
- A pull request is ``merged``. Github Webhook event (Action: ``closed``, Merged: ``True``)
- A PR Version has reached its life time.

We might want to set a life time of a PR version in case we don't receive webhook
event for a pull request being closed/merged or if a PR is stale (not merged for a long time).
We need to delete the PR Version alongside the Build data from the blob storage.
Copy link
Member

Choose a reason for hiding this comment

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

I think we can just plan to keep the PR build data forever, and deal with it in the future if we have too much. I don't think that will be too big of an issue though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay! removed this part from the design doc.


Excluding PR Versions from Search Engines
-----------------------------------------

We should Exclude the PR Versions from Search Engines because it might cause problems
for RTD users as they might land to a pull request doc but not the original Project Docs.
This will cause confusion for the users.

Serving PR Docs
---------------

We need to think about how we want to serve the PR Docs.
we could serve the PR builds using ``<pr_number>`` namespace.
We can do something like: ``https://<project_slug>.readthedocs.io/en/pr/<pr_number>/``
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking we could sever this from another domain, malicious users could use PR's like a phishing method (since it shares the same domain).

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay I'll add this to the Doc


Related Issues
--------------

- `Autobuild Docs for Pull Requests`_
- `Add travis-ci style pull request builder`_


.. _Autobuild Docs for Pull Requests: https://github.com/rtfd/readthedocs.org/issues/5684
.. _Add travis-ci style pull request builder: https://github.com/rtfd/readthedocs.org/issues/1340