-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
||
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>/`` |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
Also, you may want to follow https://docs.readthedocs.io/en/stable/docs.html#content to split lines |
@stsewd Thanks a lot for your Feedback. It really helped a lot. 👍 Updated the PR with the changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are my initial comments. Good work!
docs/design/pr-builder.rst
Outdated
- Deleting PR version and the build data | ||
- Excluding PR Versions from Search Engines | ||
- Serving PR Docs | ||
- Update the Footer API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the changes you have in mind here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that Github has an specific event/action for this. What is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its "action": "synchronize"
I'll update the PR with this
docs/design/pr-builder.rst
Outdated
|
||
- 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@humitos so you are saying that we will not delete the PR versions if closed or merged? Correct me if I'm wrong:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. we can implement this after the initial implementation is shipped.
docs/design/pr-builder.rst
Outdated
|
||
- 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>/`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Serving from another domain will be confusing
docs/design/pr-builder.rst
Outdated
- 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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 on the Doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@humitos Thank you for your feedback. I have updated the PR. I have left some questions on your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
docs/design/pr-builder.rst
Outdated
- 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Co-Authored-By: Eric Holscher <25510+ericholscher@users.noreply.github.com>
We could start by updating the |
------------------------------------------- | ||
|
||
Pull requests can be Treated as a Type of Temporary ``Version``. | ||
We might consider adding a ``VERSION_TYPES`` to the ``Version`` model. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be using the same Version model, just in a different column
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we would make the default Version
manager exclude those versions, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the PR with Versions managers (internal
, external
).
Updated the Design Doc with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I realize I'm reviewing this after it is merged but I took a look at the document and it sounds good! |
@davidfischer Thanks :) |
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