Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Builder Design Doc #5705
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
Changes from 5 commits
0628887
fff7a81
a154fea
2f34882
8198de9
29f549e
bafc77f
ad80675
c3500ef
f1ff272
b6fd291
a8c84f0
f605ba7
9f48d3b
0b1686d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
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! :)
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.
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 thisThere 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.
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.
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
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 theconf.py
(ja
, e.g) of that PR and we will can force theVersion.slug
to bepr5726
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 thispr-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>
.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.