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

Serve external version through El Proxito #6434

Merged
merged 11 commits into from Jan 6, 2020

Conversation

@humitos
Copy link
Member

humitos commented Dec 4, 2019

Allow El Proxito to serve EXTERNAL versions.

Before this PR, we were serving external version just by using an NGINX proxy_pass rule pointing to the Blob storage. This new behavior is cleaner but also required to serve these versions on corporate, since they require auth and El Proxito can handle it properly.

Steps to test this PR

Import readthedocs/test-builds into your local instance:

python manage.py import_project_from_live test-builds

Then, open a Django shell and create an external version pointing to PR 13 in that repository:

project = Project.objects.get(slug='test-builds')
version = Version.objects.create(project=project, type='external', active=True, verbose_name='13', slug='13', identifier='617672ce68a24c4397ae009a69b4d93850e4b8b6')

You can also setup a OAuth GitHub application, connect your GH account, etc but that's too complicated just for testing accessing to an external version documentation.

Finally, trigger a build for that version:

from readthedocs.core.utils import trigger_build
trigger_build(project, version)

At this point, you will be able to hit http://external-builds.community.dev.readthedocs.io/html/test-builds/13/index.html and see the documentation built from a PR properly.

@humitos humitos requested a review from ericholscher Dec 4, 2019
humitos added 4 commits Dec 4, 2019
This setting is not needed anymore since we are serving PR docs under
the same docs domain for the project.
Copy link
Member

ericholscher left a comment

This feels way cleaner than our previous approach. I really like it.

This is also blocked on shipping proxito, sadly. I think we could start routing /_/ from our docs domains to proxito if we wanted, that would let us ship this and the media downloads.

Also, the URL that it links to now is http://test-builds.community.dev.readthedocs.io/_/external/html/test-builds/13/index.html for me locally 👍

readthedocs/builds/models.py Outdated Show resolved Hide resolved
@humitos

This comment has been minimized.

Copy link
Member Author

humitos commented Dec 9, 2019

Originally, this PR was serving PR builds at a different domain than the docs domain:

At this point, you will be able to hit http://external-builds.community.dev.readthedocs.io/html/test-builds/13/index.html and see the documentation built from a PR properly.

Then we decided that it could be good to serve it under the same domain as the docs:

Also, the URL that it links to now is http://test-builds.community.dev.readthedocs.io/_/external/html/test-builds/13/index.html for me locally +1

Although, we realize that this is a potential security issue on projects with custom domains.

@humitos humitos force-pushed the humitos/serve-external-versions-proxito branch 3 times, most recently from d026eea to 0b41a1f Dec 9, 2019
readthedocs/proxito/urls.py Outdated Show resolved Hide resolved
@humitos humitos force-pushed the humitos/serve-external-versions-proxito branch 2 times, most recently from f06859c to b556625 Dec 9, 2019
Due to a potential security issue when serving the PRs under the same
domain of the docs (maybe a custom one), we need to serve them in a
different domain than the real docs.
@humitos humitos force-pushed the humitos/serve-external-versions-proxito branch from b556625 to ac14a1d Dec 9, 2019
Copy link
Member

ericholscher left a comment

This looks mergable to me now 👍

We'll need to also change the nginx configs in prod to support this, but that's good since we can easily deploy and test this.

@humitos

This comment has been minimized.

Copy link
Member Author

humitos commented Jan 6, 2020

We'll need to also change the nginx configs in prod to support this, but that's good since we can easily deploy and test this.

If I understand correctly, we can safely deploy these changes and they won't have any effect in production until we change the NGINX config file to start using El Proxito. In fact, after deploying this, it should be working in our test-builds project which has El Proxito enabled.

So, I think we are good to merge this and safely deploy in the next deploy.

@humitos humitos removed the Status: blocked label Jan 6, 2020
@humitos humitos merged commit ad6360d into master Jan 6, 2020
3 checks passed
3 checks passed
continuous-documentation/read-the-docs Read the Docs build succeeded!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pyup.io/safety-ci No dependencies with known security vulnerabilities.
Details
@humitos humitos deleted the humitos/serve-external-versions-proxito branch Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.