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
Merged

Conversation

@saadmk11
Copy link
Member

@saadmk11 saadmk11 commented May 16, 2019

This is the initial design Doc for the PR Builder. I would like to get some feedback and Guidance on this to carry on the project.
closes #5700
reference issue #5684

@saadmk11 saadmk11 requested review from safwanrahman and May 16, 2019
Copy link
Member

@stsewd stsewd left a comment

Just left some comments to consider, it looks good 👍

We also need to decide what to do with the footer api here (all versions are listed here), maybe use a visual hint that it's a PR? (we show a red footer when the version is marked as deprecated)

docs/design/pr-builder.rst Outdated Show resolved Hide resolved
docs/design/pr-builder.rst Show resolved Hide resolved

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

@stsewd stsewd May 17, 2019

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

@saadmk11 saadmk11 May 17, 2019

Okay I'll add this to the Doc

@stsewd
Copy link
Member

@stsewd stsewd commented May 17, 2019

Also, you may want to follow https://docs.readthedocs.io/en/stable/docs.html#content to split lines

@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented May 17, 2019

@stsewd Thanks a lot for your Feedback. It really helped a lot. 👍 Updated the PR with the changes.

Copy link
Member

@humitos humitos left a comment

Here are my initial comments. Good work!

- Deleting PR version and the build data
- Excluding PR Versions from Search Engines
- Serving PR Docs
- Update the Footer API
Copy link
Member

@humitos humitos May 23, 2019

What are the changes you have in mind here?

Copy link
Member Author

@saadmk11 saadmk11 May 24, 2019

@humitos we should remove the PR Versions from the version list of the Footer

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 Branch.
Copy link
Member

@humitos humitos May 23, 2019

I prefer this one.

Copy link
Member Author

@saadmk11 saadmk11 May 24, 2019

me too! :)

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.
Copy link
Member

@humitos humitos May 23, 2019

I assume that Github has an specific event/action for this. What is it?

Copy link
Member Author

@saadmk11 saadmk11 May 24, 2019

its "action": "synchronize" I'll update the PR with this


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

@humitos humitos May 23, 2019

I'd not focus too much on this one at first, since if we are going to use blob storage, we won't really care about keep the opened PRs documentation online.

Copy link
Member Author

@saadmk11 saadmk11 May 24, 2019

@humitos so you are saying that we will not delete the PR versions if closed or merged? Correct me if I'm wrong:)

Copy link
Member

@humitos humitos May 28, 2019

No. I'm saying that we should not invest time on deleting PRs after a life time reached and not merged/closed.

The first two points are correct to me.

Excluding PR Versions from Elasticsearch Indexing
-------------------------------------------------

We should exclude to PR Versions from being Indexed to Elasticsearch.
Copy link
Member

@humitos humitos May 23, 2019

I think this is the easiest way and it's fine to me.

Although, I was wondering if we want to have it indexed but excluded from the global search results. I mean, if you perform a search over a PR version documentation you probably expect that to work and return the proper results, right?

On the other hand, if you perform a search over the whole project for a particular term, you don't want to expose results from the PR versions.

What I'm saying it's probably too complicated and should not be a focus for the first implementation, though.

Copy link
Member Author

@saadmk11 saadmk11 May 24, 2019

I agree. we can implement this after the initial implementation is shipped.


- We could serve the PR Docs from another Domain.
- We could serve the PR Docs using ``<pr_number>`` namespace on the same Domain.
``https://<project_slug>.readthedocs.io/en/pr/<pr_number>/``
Copy link
Member

@humitos humitos May 23, 2019

I think that if we keep our current URLs it could work. Example, https://<project_slug>.readthedocs.io/<language_code>/<version_slug>/

This way, we will stick to the language_code defined in the conf.py (ja, e.g) of that PR and we will can force the Version.slug to be pr5726 when a version is created. The resulting URL would be https://myproject.readthedocs.io/ja/pr5726/. This will interferes with branches and tags named in the same way, though.

On the other hand, single version project URLs will be https://myproject.readthedocs.io/pr5726/, which will interferes with any folder named pr5726 in the official project docs.

Another idea could be to use https://pr.<project_slug>.readthedocs.io/<pr_number>/.

I suppose that using any other approach that does not match our current mapping or adds "an special case inside our current mapping" will be complicated to adapt around all the code we have already written.

Copy link
Member Author

@saadmk11 saadmk11 May 24, 2019

@humitos I like this one more https://myproject.readthedocs.io/ja/pr5726/. can we make the slug like this pr-5726 ?
for the second one shouldn't we have the <language_code>?
Both of them are good ideas. I'll add them in the Docs 👍

Copy link
Member

@humitos humitos May 28, 2019

In the second case there is no reason to add the language code in the URL from my point of view.

In the first case, it's useful to keep compatibility with the rest of URL handling and just base the PR serving on the slug being pr-<number>.


We need to think about how we want to serve the PR Docs.

- We could serve the PR Docs from another Domain.
Copy link
Member

@humitos humitos May 23, 2019

Serving from another domain will be confusing

- We could serve the PR Docs using ``<pr_number>`` namespace on the same Domain.
``https://<project_slug>.readthedocs.io/en/pr/<pr_number>/``

Update the Footer API
Copy link
Member

@humitos humitos May 23, 2019

I would not add this from the footer API and use an sphinx extension that we force to install on these version to add a banner similar to the warning banner mentioning that this is a draft or similar.

Copy link
Member Author

@saadmk11 saadmk11 May 24, 2019

Okay, I'll add this on the Doc

Copy link
Member Author

@saadmk11 saadmk11 May 24, 2019

I'm not familiar with sphinx extensions. I would like to know more and carry on this discussion on this in a new issue when we get close to implementing this.

@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented May 24, 2019

@humitos Thank you for your feedback. I have updated the PR. I have left some questions on your comments.

Copy link
Member

@ericholscher ericholscher left a comment

Lots of good stuff here. It feels like we're ready to get started to me.

@saadmk11 Do you already know where you want to start, or should we talk a bit more about that to figure it out?

- We don't receive webhook event for a pull request being closed/merged
- 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

@ericholscher ericholscher May 29, 2019

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

@saadmk11 saadmk11 May 30, 2019

Okay! removed this part from the design doc.

saadmk11 and others added 2 commits May 30, 2019
Co-Authored-By: Eric Holscher <25510+ericholscher@users.noreply.github.com>
@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented May 30, 2019

@saadmk11 Do you already know where you want to start, or should we talk a bit more about that to figure it out?

We could start by updating the Version model and all the version Querysets to reflect that.

-------------------------------------------

Pull requests can be Treated as a Type of Temporary ``Version``.
We might consider adding a ``VERSION_TYPES`` to the ``Version`` model.
Copy link
Member

@stsewd stsewd Jun 3, 2019

Thinking more about this, any reason why we want to merge the PR type with the "normal" ones?

Feels like more work trying to exclude all Pull Request versions, instead we could just have something like Project.temporal_versions

I can't think of a case where we want this type listed in the same place with the normal PRs.

Copy link
Member

@stsewd stsewd Jun 3, 2019

This would be using the same Version model, just in a different column

Copy link
Member

@ericholscher ericholscher Jun 3, 2019

Then we would make the default Version manager exclude those versions, correct?

Copy link
Member Author

@saadmk11 saadmk11 Jun 4, 2019

Updated the PR with Versions managers (internal, external).

@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented Jun 8, 2019

Updated the Design Doc with PR Builds Admin tab in the Project Dashboard proposal.

Copy link
Member

@ericholscher ericholscher left a comment

I'd like to get this merged, since we are now working on it. I think we're pretty good, and we can update it as we change things during implementation.

@ericholscher ericholscher merged commit d36d453 into readthedocs:master Jun 12, 2019
1 check was pending
@saadmk11 saadmk11 deleted the pr-builder-doc branch Jun 12, 2019
@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Jun 12, 2019

I realize I'm reviewing this after it is merged but I took a look at the document and it sounds good!

@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented Jun 13, 2019

@davidfischer Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants