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 non-html at documentation domain though El Proxito #6419

Merged
merged 39 commits into from Jan 16, 2020

Conversation

humitos
Copy link
Member

@humitos humitos commented Nov 28, 2019

Download URLs from the footer call will be like,

Once the user hit these URLs the PDF/ePub/HTML will be serve directly at that URL by El Proxito --no redirect is performed.

This PR also includes a change in the El Proxito logic to serve any file as well. Instead of generating the path to the filename manually, the django-storage backend is used. This allow us to generate the same URL for non-html files from the FooterHTML view than from ServeDocsBase.

Also, the "Downloads" link that point to the Dashboard in the footer is removed.

Closes #5855
Closes #6100

humitos added 2 commits Nov 28, 2019
Download URLs from the footer call will be like,

- project-slug.readthedocs.io/_/downloads/pdf/project-slug/latest/
- docs.example.org/_/downloads/pdf/subproject-slug/latest/

Once the user hit these URLs the PDF/ePub/HTML will be serve directly
at that URL by El Proxito --no redirect is performed.

This commit also includes a change in the El Proxito logic to serve
any file as well. Instead of generating the `path` to the filename
manually, the django-storage backend is used. This allow us to
generate the same URL for non-html files from the `FooterHTML` view
than from `ServeDocsBase`.
@humitos humitos requested review from ericholscher and davidfischer and removed request for ericholscher Nov 28, 2019
docker/nginx/proxito.conf Outdated Show resolved Hide resolved
docker/nginx/web.conf Outdated Show resolved Hide resolved
readthedocs/proxito/views/serve.py Outdated Show resolved Hide resolved
readthedocs/proxito/views/serve.py Outdated Show resolved Hide resolved
readthedocs/urls.py Outdated Show resolved Hide resolved
Copy link
Member

@ericholscher ericholscher left a comment

This is a good start, but again we need to not break backwards compat with these changes.

readthedocs/api/v3/serializers.py Show resolved Hide resolved
readthedocs/projects/urls/public.py Show resolved Hide resolved
readthedocs/proxito/views/serve.py Outdated Show resolved Hide resolved
readthedocs/settings/test.py Outdated Show resolved Hide resolved
readthedocs/urls.py Outdated Show resolved Hide resolved
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Dec 2, 2019

I'm wondering if it would be easier to just support both the dashboard & in-doc download URL's for a time. It seems like we're adding a lot of complexity here, when we really just want to be able to support both for a while. Once we have both working, we can figure out how to migrate the API and other paths we link to.

Copy link
Contributor

@davidfischer davidfischer left a comment

I'm wondering if it would be easier to just support both the dashboard & in-doc download URL's for a time.

I'm in favor of this because I suspect some people are linking to the old URL.

readthedocs/settings/base.py Show resolved Hide resolved
@humitos
Copy link
Member Author

@humitos humitos commented Dec 3, 2019

@davidfischer @ericholscher I'm not sure to follow you. What are the changes I need to do in this PR to be accepted?

You both mentioned supporting both, old and new links. This PR won't change that after reverting the commit that removed the URL defined. On the other hand, if you are talking about the NGINX config (#6419 (comment)), I'm not sure it worth to have this in development since the only way to get there is manually writing the URL. We won't change this in the -ops repo.

@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Dec 3, 2019

What are the changes I need to do in this PR to be accepted?

I think we were both saying that the old URL (eg. https://readthedocs.org/projects/project-slug/downloads/pdf/latest/) should still work. Since we put that back and just removed the name= parameter, I think we should be ok.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Dec 4, 2019

Yea, as long as we don't break the old URL's I'm happy.

Copy link
Member

@ericholscher ericholscher left a comment

This is going in a good direction, but I still think the URLConf hack is the wrong approach. Let's either keep serving from the dashboard URL's for now, or just explicitly build the URL, instead of the current hack of adding a URLConf to the main site that we never want to serve.

docker/nginx/proxito.conf Outdated Show resolved Hide resolved
docker/nginx/web.conf Outdated Show resolved Hide resolved
readthedocs/api/v2/templates/restapi/footer.html Outdated Show resolved Hide resolved
readthedocs/projects/models.py Outdated Show resolved Hide resolved
readthedocs/projects/urls/public.py Show resolved Hide resolved
@ericholscher ericholscher changed the title Serve non-html at documentation domain Serve non-html at documentation domain though El Proxito Dec 5, 2019
humitos added 3 commits Dec 5, 2019
There is no need for `full_path` attribute.

It was not used anywhere and the path without the domain does not
make sense and will be broken.
@ericholscher ericholscher removed the Status: blocked label Dec 16, 2019
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Dec 16, 2019

This feels close, but I still don't like that we're serving the URL's on all proxito domains. Locally this works:

Downloads the test-builds PDF on the test-utils domain :/

This all leads back to the weirdness around URL's and domains, but this is definitely not ready to ship as it is currently implemented. We need to have a better way of download this project's PDF, not just ship our old general "download any PDF endpoint" onto the doc domains.

readthedocs/projects/models.py Outdated Show resolved Hide resolved
@humitos humitos added the Status: blocked label Dec 17, 2019
@humitos
Copy link
Member Author

@humitos humitos commented Dec 17, 2019

The code looks good and I think we agree we want this. Although, I'm blocking it because I suppose we want to have a discussion/talk about how the URLs will look like. We've talked already about this but we didn't make a decision yet.

@humitos humitos added Accepted PR: work in progress and removed Status: blocked labels Jan 13, 2020
@humitos
Copy link
Member Author

@humitos humitos commented Jan 13, 2020

This PR is ready for re-review.

We decided to use these URLs to serve files for download:

  • docs.example.com/_/downloads/<lang>/<ver>/pdf/
  • docs.example.com/_/downloads/<alias>/<lang>/<ver>/pdf/

The latest commits implement this.

@humitos humitos removed the PR: work in progress label Jan 13, 2020
Copy link
Member

@ericholscher ericholscher left a comment

Looks like a good change, I think we're pretty much ready once the tests pass and we add a couple docstrings. 👍

readthedocs/projects/models.py Show resolved Hide resolved
readthedocs/projects/models.py Show resolved Hide resolved
readthedocs/projects/views/public.py Show resolved Hide resolved
readthedocs/proxito/urls.py Outdated Show resolved Hide resolved
@humitos
Copy link
Member Author

@humitos humitos commented Jan 16, 2020

I'm merging this PR now. Downloads are ready to start being served by El Proxito. I've already opened the -ops PRs needed together with this change for the deploy.

@humitos humitos merged commit d4e3538 into master Jan 16, 2020
3 checks passed
@humitos humitos deleted the humitos/serve-non-html-same-domain branch Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants