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

Initial attempt to serve PR builds at readthedocs.build #6629

Merged
merged 17 commits into from Feb 17, 2020

Conversation

ericholscher
Copy link
Member

@ericholscher ericholscher commented Feb 5, 2020

This updates the logic so that we can serve docs from a URL that looks like this:

http://test-builds--13.dev.readthedocs.build/en/13/

We will take the Project & Version from the domain, and then the rest of the URL will be parsed normally.

This updates the logic so that we can serve docs from a URL that looks like this:

http://test-builds--13.dev.readthedocs.build/en/13/

We will take the Project & Version from the domain, and then the rest of the URL will be parsed normally.
@ericholscher ericholscher requested a review from humitos Feb 5, 2020
Copy link
Member

@humitos humitos left a comment

It's looking good!

I think this URL won't be needed anymore if we approve this changes:

# External versions
# (RTD_EXTERNAL_VERSION_DOMAIN/html/<project-slug>/<version-slug>/<filename>)
# NOTE: requires to be before single version
url(
(
r'^html/(?P<project_slug>{project_slug})/'
r'(?P<version_slug>{version_slug})/'
r'(?P<filename>{filename_slug})'.format(
**pattern_opts,
)
),
ServeDocs.as_view(version_type=EXTERNAL),
name='docs_detail_external_version',
),

readthedocs/core/resolver.py Outdated Show resolved Hide resolved
readthedocs/core/resolver.py Show resolved Hide resolved
readthedocs/core/resolver.py Outdated Show resolved Hide resolved
readthedocs/projects/tasks.py Show resolved Hide resolved
readthedocs/proxito/middleware.py Outdated Show resolved Hide resolved
readthedocs/proxito/views/serve.py Outdated Show resolved Hide resolved
@humitos
Copy link
Member

@humitos humitos commented Feb 6, 2020

http://test-builds--13.dev.readthedocs.build/en/13/

we are going to use a different domain for corporate, correct?

@agjohnson agjohnson added the PR: work in progress label Feb 10, 2020
Copy link
Member

@humitos humitos left a comment

I was able to review this more deeply. I like the approach taken here, although I'd like to polish this code a little more to follow the pattern we have and improve naming. This code is getting hard to follow because we need to have a lot of things in mind to understand it, so the simpler we can make it the better.

I made some suggestions to make it a little simpler, IMHO but I think it's pretty close to be ready.

readthedocs/proxito/views/mixins.py Show resolved Hide resolved
readthedocs/core/resolver.py Outdated Show resolved Hide resolved
readthedocs/projects/models.py Show resolved Hide resolved
readthedocs/proxito/middleware.py Show resolved Hide resolved
readthedocs/proxito/middleware.py Show resolved Hide resolved
readthedocs/proxito/views/mixins.py Outdated Show resolved Hide resolved
readthedocs/proxito/views/mixins.py Outdated Show resolved Hide resolved
readthedocs/proxito/views/serve.py Show resolved Hide resolved
readthedocs/proxito/views/serve.py Show resolved Hide resolved
readthedocs/proxito/views/utils.py Outdated Show resolved Hide resolved
@humitos
Copy link
Member

@humitos humitos commented Feb 11, 2020

I understand that the URL for production will be like http://test-builds--13.org.readthedocs.build/en/13/.

Since we have the canonical project's slug in the host, having a lang in the path is also irrelevant --we won't have a URL like http://test-builds--13.org.readthedocs.build/ja/13/. In case of Japanese, it will have another project slug, like http://test-builds-ja--13.org.readthedocs.build/ja/13/ and we end up with duplicated lang and version in this case.

@humitos
Copy link
Member

@humitos humitos commented Feb 11, 2020

I pulled down the PR and it works properly locally. Although, I'm not sure why it's not required to add a server_name *.dev.readthedocs.build in our proxito nginx conf.

@ericholscher
Copy link
Member Author

@ericholscher ericholscher commented Feb 13, 2020

I pulled down the PR and it works properly locally. Although, I'm not sure why it's not required to add a server_name *.dev.readthedocs.build in our proxito nginx conf.

Because nginx always has a default server, so it's just handling it properly when it gets hit. I've made it explicit here.

dockerfiles/nginx/proxito.conf Outdated Show resolved Hide resolved
readthedocs/core/resolver.py Outdated Show resolved Hide resolved
readthedocs/projects/models.py Show resolved Hide resolved
readthedocs/proxito/views/serve.py Show resolved Hide resolved
Copy link
Member

@humitos humitos left a comment

Looks good to me when we figure it out why tests are failing.

@ericholscher ericholscher merged commit cd2bee3 into master Feb 17, 2020
3 checks passed
@ericholscher ericholscher deleted the external-builds-new-domain branch Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants